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

Fixes #127 #138

Merged
merged 3 commits into from
Nov 25, 2016
Merged

Fixes #127 #138

merged 3 commits into from
Nov 25, 2016

Conversation

EugeneZ
Copy link
Contributor

@EugeneZ EugeneZ commented Nov 24, 2016

This fixes the misalignment of the sourcemaps. However, two problems remain:

  1. Instead of using new Function(...) I had to use eval. I couldn't figure out a way to get the Function constructor to read a sourcemap, probably because sourcemaps need to be at the end of a file. There's no way to do that with a Function constructor, though I'd love to be wrong. The problem is that with eval we lose the scoping advantages of Function. That means the modules have access to all the local vars from the loader function. The way to fix this is to sideload the loadAsModule method. Are there are other side effects of this change I haven't noticed?

  2. When a new module is loaded to replace an old one, the sourcemap is discarded because the source file has the same name. The workaround here is to make the name unique somehow, and luckily we have a hash right ready for this purpose already! I already use the absolute filename for the sourcemap so that the whole -t [ babelify --absoluteSourceMaps] thing isn't needed. We need to figure out the best way to make this work with the hash. I'm thinking we figure out the cwd and replace it with the hash, so that /home/username/project/src/filename.js becomes abcdef1234567890/src/filename.js and so on with different hashes for subsequent revisions.

Despite these problems I'm opening the PR to start a discussion. I plan on addressing these two issues as described unless someone has a suggestion.

Sourcemap issues fixed
@EugeneZ EugeneZ mentioned this pull request Nov 24, 2016
@EugeneZ
Copy link
Contributor Author

EugeneZ commented Nov 24, 2016

Okay, I had some extra time and just fixed both above issues as described. Should be all set?

@milankinen
Copy link
Owner

Oh sweet thank you!!! 😍 ❤️

I change base to develop, do some minor code style changes / re-arrangements and then merge this to master.

@milankinen milankinen changed the base branch from master to development November 25, 2016 12:50
@milankinen milankinen changed the base branch from development to master November 25, 2016 12:51
@milankinen milankinen changed the base branch from master to development November 25, 2016 12:51
@milankinen milankinen merged commit 158f8cd into milankinen:development Nov 25, 2016
@milankinen
Copy link
Owner

And in 3.1.1!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants