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

Properly support sourcemaps when using the transform option #316

Merged
merged 4 commits into from Mar 4, 2015

Conversation

@ldesplat
Copy link
Contributor

ldesplat commented Mar 2, 2015

Multiple changes:

  1. Fix the command line sourcemaps option to use alias so we can finally call it with -S and --sourcemaps as documented in README
  2. Pass filename to the exported transform method (2nd parameter) so it is backwards compatible. Sourcemaps do need that information to generate nice output.
  3. Sourcemaps are now transparently handled when using transforms. It was broken before but I did not realize as my transforms always gave me 1 to 1 mappings.

Issues:

  • Testability: Unfortunately, to get code coverage for (3), I need to "install" an option to source-map-support module and I cannot do so in a way that can be undone or clean with the way things are done right now. Doing so, in tests, breaks Lab reporting.
  • I can definitely write tests that will cover the the lines of code (by invoking the cli directly as in cli.js) but we will not get code coverage reports for that.

Question:

  • When Lab crashes, is there a way to see the stack trace. It really likes to eat its own errors...

I apologize for the sourcemap support :(

@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 2, 2015

@ldesplat the code coverage will definitely be needed to merge this. For the crash issue, can you create a PR with a test that demonstrates what you mean?

Thanks for doing all of the hard work on this feature, it will be very useful for a lot of people.

@geek geek self-assigned this Mar 2, 2015
@geek geek added the feature label Mar 2, 2015
@geek geek added this to the 5.3.1 milestone Mar 2, 2015
@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Mar 3, 2015

100% code coverage and a feature that works in all of my projects.

The issue now, is just that I have not been able to write an automated Lab test that tests it a to z. I tried by doing a test in test/cli.js but none of the current tests try to do coverage analysis, and when I do I always get 0/0 coverage.

So, we get a nice 100% number but no meaningful tests. That's the best I could do...

@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 3, 2015

@ldesplat nice work!

});
};

exports.retrieveFile = function (path) {

This comment has been minimized.

Copy link
@geek

geek Mar 3, 2015

Member

whats the reason to duplicate this function here and in coverage... would it be safe to have a single version used everywhere?

This comment has been minimized.

Copy link
@ldesplat

ldesplat Mar 3, 2015

Author Contributor

@geek It would be preferable yes. Now, that I look at it it seems it could be possible. I will have to move internals.transform in coverage.js and externalize it in transform.js. Maybe some other changes as well. I'll look into it.

I initially did it because I am storing all the transformed files in a cache (in internals.fileCache for transform.js and coverage.js), so that I can give it to source-map-support. Otherwise, source-map-support would read the file from the disk and it would not have the sourcemap available.

@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 3, 2015

I don't see any, but do you think this PR has any breaking changes? @ldesplat

@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Mar 3, 2015

There should no breaking changes. The API has changed slightly but it is adding a parameter to the transform method, so it's backwards compatible.

It also does give different output when using sourcemaps & transform since now it will be correct.. :)

@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Mar 3, 2015

Rebased on top of Lab's new changes. Should be ready to go :)

@ldesplat ldesplat mentioned this pull request Mar 4, 2015
geek added a commit that referenced this pull request Mar 4, 2015
Properly support sourcemaps when using the transform option
@geek geek merged commit c2179f3 into hapijs:master Mar 4, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.