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

perf: replace source-map package with @jridgewell/trace-mapping #489

Merged
merged 1 commit into from Jun 1, 2022

Conversation

onigoetz
Copy link
Contributor

@onigoetz onigoetz commented Jun 1, 2022

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

The trace-mapping module is faster than source-map in every benchmark
https://github.com/jridgewell/trace-mapping#benchmarks

It's also being adopted by many packages across the ecosystem (Jest, Babel, Terser at least have included it already)
And it doesn't need to be async like source-map 0.7.*

A previous PR to upgrade for source-map 0.7 was refused in the past : #280

Breaking Changes

None

Additional Info

@alexander-akait
Copy link
Member

@onigoetz I think we need to change it in more places, we don't think it will improve somthing, because we use source maps here only for error reporting

@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #489 (0e2ce1b) into master (66fdec8) will not change coverage.
The diff coverage is 60.00%.

@@           Coverage Diff           @@
##           master     #489   +/-   ##
=======================================
  Coverage   97.11%   97.11%           
=======================================
  Files           3        3           
  Lines         312      312           
  Branches      114      114           
=======================================
  Hits          303      303           
  Misses          9        9           
Impacted Files Coverage Δ
src/utils.js 95.00% <ø> (ø)
src/index.js 97.34% <60.00%> (ø)

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 66fdec8...0e2ce1b. Read the comment docs.

@onigoetz
Copy link
Contributor Author

onigoetz commented Jun 1, 2022

I understand that in this library's case it won't impact performance nor package size greatly

However there seems to be a big interest to move away from source-map and source-map-js towards these packages and I think that every package adopting this newer alternative is a move towards deprecating an unmaintained package and at some point remove it from our dependency trees entirely

@alexander-akait
Copy link
Member

@onigoetz I agree, let's migrate, I think we should do the same in css-minimizer-webpack-plugin and maybe somewhere else? 🤔

@alexander-akait alexander-akait merged commit 6020a94 into webpack-contrib:master Jun 1, 2022
@alexander-akait
Copy link
Member

Thanks

@onigoetz
Copy link
Contributor Author

onigoetz commented Jun 1, 2022

I personally did a yarn why source-map on my main project and am trying to create PRs here and there :)

@alexander-akait
Copy link
Member

Feel free to send PRs in other repos 👍

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.

None yet

2 participants