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

Uglify sourcemaps support #617

Merged
merged 21 commits into from
Feb 21, 2018
Merged

Uglify sourcemaps support #617

merged 21 commits into from
Feb 21, 2018

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented Jan 22, 2018

  • Sourcemap support for builds (supporting uglify)
  • Added functions to search closest mapping for both original and generated mappings
  • Tried to make it as fast as possible (Based on binary search) 🚀

@codecov-io
Copy link

codecov-io commented Jan 22, 2018

Codecov Report

Merging #617 into master will decrease coverage by 0.89%.
The diff coverage is 26%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #617     +/-   ##
=========================================
- Coverage   91.98%   91.08%   -0.9%     
=========================================
  Files          68       68             
  Lines        3404     3453     +49     
=========================================
+ Hits         3131     3145     +14     
- Misses        273      308     +35
Impacted Files Coverage Δ
src/Bundler.js 92.47% <ø> (ø) ⬆️
src/transforms/uglify.js 70.27% <57.89%> (-17.97%) ⬇️
src/SourceMap.js 57.29% <6.45%> (-23.31%) ⬇️
src/utils/installPackage.js 95.55% <0%> (+4.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d9d14c...59436cd. Read the comment docs.

sourceMap: asset.options.sourceMaps
? {
filename: asset.relativeName
}
Copy link
Member

Choose a reason for hiding this comment

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

I think uglify has a way to pass an input source map to it so you don't have to manually map it like you did below. See https://github.com/mishoo/UglifyJS2#source-map-options. Looks like the content option?

Copy link
Member Author

@DeMoorJasper DeMoorJasper Jan 22, 2018

Choose a reason for hiding this comment

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

I've seen it but that would add an extra stringify (asset.sourcemap.toString()) and parse (on uglify's side) operation to the building of the new sourcemap, which is the largest performance problem with source-map 0.6.1

@sant123
Copy link
Contributor

sant123 commented Feb 7, 2018

¿Any advance on this? Just asking 😝

@DeMoorJasper
Copy link
Member Author

@sant123 as far as i know it's ready to go, just waiting on the approval and merge from Devon

@DeMoorJasper
Copy link
Member Author

@devongovett this should be ready to go

}
}

// Log all warnings
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not log all of these. Uglify logs a lot of warnings and they are pretty much useless. I removed this in master already.

if (result.error) {
throw result.error;
}

if (result.map) {
result.map = await new SourceMap().addMap(JSON.parse(result.map));
Copy link
Member

Choose a reason for hiding this comment

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

I think it's possible to get the map without converting to JSON first. https://github.com/mishoo/UglifyJS2/blob/master/lib/sourcemap.js#L94

Might be faster - maybe you could even access the original mappings from there so you don't need to decode it.

@devongovett devongovett merged commit d9f3c25 into master Feb 21, 2018
@devongovett devongovett deleted the feature/uglify-sourcemaps branch February 21, 2018 18:47
devongovett pushed a commit that referenced this pull request Oct 15, 2018
devongovett pushed a commit that referenced this pull request Oct 15, 2018
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.

None yet

4 participants