-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
JS Source Maps support #506
JS Source Maps support #506
Conversation
Add new improvements
Add all new commits
new features
new features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start! Thanks for working on this. 🎉
src/packagers/JSPackager.js
Outdated
@@ -20,6 +21,7 @@ class JSPackager extends Packager { | |||
async start() { | |||
this.first = true; | |||
this.dedupe = new Map(); | |||
this.sourcemapPackager = new SourcemapPackager(this.bundle, this.bundler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you register the SourcemapPackager in here you won't need to construct it from here. addAsset
will automatically get called as well, you don't need to call it from here. This way the source map packager will automatically work for non-JS langs as well if we generate maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do i go about this without overwriting the jsPackager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd register for type map
not type js
so it shouldn't overwrite.
this.add('map', SourceMapPackager);
Parcel should already be creating a bundle of type map
since you return one from JSAsset#generate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried this out and it appears bundles are created based on asset.type not asset.generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right, I changed this recently. See https://github.com/parcel-bundler/parcel/blob/master/src/Bundler.js#L413-L420. Looks like your branch doesn't have that change.
src/packagers/JSPackager.js
Outdated
await this.dest.end('},{},' + JSON.stringify(entry) + ')'); | ||
await this.dest.write('},{},' + JSON.stringify(entry) + ')'); | ||
await this.dest.write( | ||
`\n//# sourceMappingURL=${this.sourcemapPackager.url}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you won't have access to the source map packager, the URL will be the name of the JS bundle with .js
replaced with .map
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, will fix this
src/assets/JSAsset.js
Outdated
@@ -116,7 +116,8 @@ class JSAsset extends Asset { | |||
} | |||
|
|||
return { | |||
js: code | |||
js: code, | |||
map: this.sourcemap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll also need to generate 1:1 source maps for files that we don't process with babel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/packagers/SourcemapPackager.js
Outdated
column: mapping.originalColumn | ||
}, | ||
generated: { | ||
line: mapping.generatedLine, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we need to offset this line by the current line number in the file since we're concatenating a bunch of things together? and what about the extra lines added by the JSPackager... hmm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should, i'll dive deeper into this
Add new features from master branch
…into feature/source-maps
@devongovett I got the big portions of source maps done, my main concern now is the offsets. constructor() {
...
this.offsets = new Map();
}
addOffset(assetname, line, column) {
this.offsets.set(assetname, {line, column});
} And this to addAsset(asset) {
...
this.bundle.addOffset(asset.relativename, this.lineCount, 0);
this.writeModule(...);
...
} This way i could just extract the perfectly accurate offsets from the bundle, this would also support css in the future, if cssPackager just adds offsets as well. The current issue with this is that the map packager is triggered before the js packager, which would require a few edits before this to work, so i'm wondering if this is the best approach before changing/adding all this code and if you have any tips on how to approach triggering the packaging of |
…into feature/source-maps
@devongovett With the env issue I commented above needing clearing of cache, something you should catch and show a message? Error handling in general ... :) |
@DeMoorJasper Did a git pull on your fork and rm node_modules and reinstall and seems to have sorted my issues at quick glance. Thanks. |
After people have mentioned the new improved source-map and reading a blog post about the many performance improvements i decided to branch of this pr and make a source-map 0.7 compatible version for when it lands: https://github.com/DeMoorJasper/parcel/tree/source-map-0.7 EDIT: |
source-maps 0.7 update
…into feature/source-maps
One problem: source-map 0.7 requires Node 8. I know we're not there yet, but I think we are aiming to support Node 6 in parcel as well... |
@devongovett Should i downgrade back to source-map 0.6.1? I see no way around this as webassembly is not supported below node.js 8.0 |
Yeah for now. Filed an issue to see if they can include the JS only version for Node 6 users: mozilla/source-map#313 |
src/assets/JSAsset.js
Outdated
if (this.globals.size > 0) { | ||
code = Array.from(this.globals.values()).join('\n') + '\n' + code; | ||
if (this.options.sourcemaps) { | ||
if (!SourceMap.isSourceMapInstance(this.sourcemap)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not this.sourcemap instanceof SourceMap)
? The current test for sources.length
seems like it might be somewhat brittle.
src/assets/JSAsset.js
Outdated
if (this.options.sourcemaps) { | ||
if (!SourceMap.isSourceMapInstance(this.sourcemap)) { | ||
let sourcemap = new SourceMap(); | ||
if (!this.sourcemap.version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this check for? When will a sourcemap not have a version
?
src/SourceMap.js
Outdated
} | ||
|
||
isConsumer(map) { | ||
return map && map.computeColumnSpans; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just do map instanceof sourceMap.SourceMapConsumer
? This check currently seems somewhat brittle since they could change the internal implementation.
Speeds things up slightly in JSPackager since we can use the line count calculated in JSAsset rather than re-computing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing work @DeMoorJasper! 🥇 Only refactored a few very minor things. The performance work you did in particular was very impressive. 🔥
Going to merge! Let's follow up with production support in a separate PR (will require uglify sourcemap support). 🚀
* run prettier * initial sourcemap * improve sourcemap constructor * also support SourceMapConsumer as addMap input * some cleanup and fixes * add lineOffset and 1:1 sourcemaps * add lineOffset and 1:1 sourcemaps * slight sourcemaputil improvement * use babel-generator instead of double generating * add 1:1 sourcemap for untranspiled assets * update offset * fix source paths * Better offset solution * fix sourceContent not being set by babel-generator * fix small offset bug * add sourcemap option and offset when globals are added * add cli option to disable sourcemaps * ts & coffeescript support * fix most tests * All tests (except hmr) fixed + bugfix for ts * fix hmr tests * comment out error throwing in tests * fix windows tests * add sourcemap tests * add comment to why throwing errors is commented out in tests * update with circular fix, now throws on test failures * fix generator for invalid maps * fix tests * fix tiny issues * small performance improvement * rewrite sourcemap handling, less generator constructing overhead and some more improvements * extendSourceMap bugfix * Use babel rawMappings to remove encoding step * small performance fixes * improve linecounter * small performance improvement and bugfix * small improvement * change for source-map 0.7 compatibility, improves performance a lil * switch to official npm release * tiny improvement * only install source-map 0.7 if possible * remove optional 0.7 dep * Minor refactorings * Store precomputed line count as part of source map * Clean up * Last cleanup
* run prettier * initial sourcemap * improve sourcemap constructor * also support SourceMapConsumer as addMap input * some cleanup and fixes * add lineOffset and 1:1 sourcemaps * add lineOffset and 1:1 sourcemaps * slight sourcemaputil improvement * use babel-generator instead of double generating * add 1:1 sourcemap for untranspiled assets * update offset * fix source paths * Better offset solution * fix sourceContent not being set by babel-generator * fix small offset bug * add sourcemap option and offset when globals are added * add cli option to disable sourcemaps * ts & coffeescript support * fix most tests * All tests (except hmr) fixed + bugfix for ts * fix hmr tests * comment out error throwing in tests * fix windows tests * add sourcemap tests * add comment to why throwing errors is commented out in tests * update with circular fix, now throws on test failures * fix generator for invalid maps * fix tests * fix tiny issues * small performance improvement * rewrite sourcemap handling, less generator constructing overhead and some more improvements * extendSourceMap bugfix * Use babel rawMappings to remove encoding step * small performance fixes * improve linecounter * small performance improvement and bugfix * small improvement * change for source-map 0.7 compatibility, improves performance a lil * switch to official npm release * tiny improvement * only install source-map 0.7 if possible * remove optional 0.7 dep * Minor refactorings * Store precomputed line count as part of source map * Clean up * Last cleanup
Adds source map support to Javascript, Typescript & Coffeescript.
Performance impact appears to be very limited to my surprise (as every transpiler was probably already creating a sourcemap in the background)
Closes #68
TODO:
Generate custom source maps instead of using babel (so we can continu using babylon)EDIT: Used babel-generator as it's already being used