Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve source map support #422

Closed
wants to merge 1 commit into from

Conversation

kpdecker
Copy link
Contributor

Attempt to resolve #274.

This adds a couple of different features:

  1. Source maps are now aware of existing source maps and will merge these with the istanbul generated source map.
  2. Adds option to include source maps inline in the instrumented output.
  3. Adds source map error support under node when using run-with-cover.

Net effect is this:

  1) parser parsers partial blocks:
     TypeError: Cannot read property 'line' of undefined
      at Object.prepareProgram (./dist/cjs/handlebars/compiler/helpers.js:9:8043)
      at Object.anonymous (./dist/cjs/handlebars/compiler/parser.js:9:3727)
      at Object.parse (./dist/cjs/handlebars/compiler/parser.js:9:37453)
      at HandlebarsEnvironment.parse (./dist/cjs/handlebars/compiler/base.js:9:3159)
      at astFor (./spec/parser.js:7:26)
      at Context.<anonymous> (./spec/parser.js:117:12)

From this:

  1) parser parsers partial blocks:
     TypeError: Cannot read property 'line' of undefined
      at Object.prepareProgram (lib/handlebars/compiler/helpers.js:135:17)
      at Object.anonymous (lib/handlebars/compiler/parser.js:16:20)
      at Object.parse (lib/handlebars/compiler/parser.js:263:36)
      at HandlebarsEnvironment.parse (./dist/cjs/handlebars/compiler/base.js:9:3159)
      at astFor (./spec/parser.js:7:26)
      at Context.<anonymous> (./spec/parser.js:117:12)

This could use additional test coverage for the source-map-support integration, but I wanted to get this out there for feedback before investing too much time in that implementation.

@@ -166,6 +172,8 @@ function run(args, commandName, enableHooks, callback) {
postLoadHookFile = path.resolve(postRequireHook);
} else if (opts.yui) { //EXPERIMENTAL code: do not rely on this in anyway until the docs say it is allowed
postLoadHookFile = path.resolve(__dirname, '../../util/yui-load-hook');
} else if (opts['source-map']) {
hookOpts.postLoadHook = require('../../util/source-map-support')(instrumenter, matchFn);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of this being handled differently vs. the other post hooks, but there was a need to access the instrumenter instance and changed the hook API seemed like it would be even more problematic.

@kpdecker kpdecker force-pushed the source-map-errors branch 2 times, most recently from c55ddad to a3416ca Compare August 14, 2015 18:20
@kpdecker
Copy link
Contributor Author

The run-with-cover implementation had a timing issue that could result in source maps being incorrectly applied to files. I've reworked this and rebased to avoid this issue.

@tilgovi
Copy link

tilgovi commented Oct 12, 2015

This is pretty nice. It's very similar to what this did for karma-coverage.

I think it needs to be combined with what's on the source-map branch right now.

I'm using karma and browserify with an ES6 Babel project so I use browserify-istanbul. Since babelify is handling the first step and outputing inline source maps (like all browserify transforms), istanbul needs to read the inline source map and output an inline source map.

  • With this PR, the inline source map to be rewritten, but the coverage report is not transformed properly and the line coverage counts look off.
  • With the source-map branch, the inline source map is not rewritten, but the coverage report is correct.

Combining these I think we could have correct coverage and source maps in the browser when debugging tests that are built with browserify.

This adds a couple of different features:

1. Source maps are now aware of existing source maps and will merge these with the istanbul generated source map.
2. Adds option to include source maps inline in the instrumented output.
3. Adds source map error support under node when using run-with-cover.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All stack traces point to line 9
3 participants