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

please update source-map to 0.7 #34

Closed
pravi opened this issue Jan 30, 2018 · 17 comments
Closed

please update source-map to 0.7 #34

pravi opened this issue Jan 30, 2018 · 17 comments

Comments

@pravi
Copy link

pravi commented Jan 30, 2018

With source-map 0.7, two tests are failing.

mocha -R spec


  ConcatSource
    ✓ should concat two sources
    ✓ should be able to handle strings for all methods

  PrefixSource
    ✓ should prefix a source
    ✓ should have consistent source/sourceAndMap behavior

  ReplaceSource
    1) should replace correctly
    2) should replace multiple items correctly

  package-entry
    ✓ should not throw SyntaxError


  5 passing (68ms)
  2 failing

  1) ReplaceSource
       should replace correctly:
     TypeError: (intermediate value).originalPositionFor is not a function
      at ReplaceSource._replacementToSourceNode (lib/ReplaceSource.js:156:54)
      at ReplaceSource.<anonymous> (lib/ReplaceSource.js:83:37)
      at Array.forEach (<anonymous>)
      at ReplaceSource.node (lib/ReplaceSource.js:76:21)
      at ReplaceSource.proto.sourceAndMap (lib/SourceAndMapMixin.js:30:18)
      at Context.<anonymous> (test/ReplaceSource.js:31:26)
      at callFn (/usr/lib/nodejs/mocha/lib/runnable.js:354:21)
      at Test.Runnable.run (/usr/lib/nodejs/mocha/lib/runnable.js:346:7)
      at Runner.runTest (/usr/lib/nodejs/mocha/lib/runner.js:442:10)
      at /usr/lib/nodejs/mocha/lib/runner.js:560:12
      at next (/usr/lib/nodejs/mocha/lib/runner.js:356:14)
      at /usr/lib/nodejs/mocha/lib/runner.js:366:7
      at next (/usr/lib/nodejs/mocha/lib/runner.js:290:14)
      at Immediate.<anonymous> (/usr/lib/nodejs/mocha/lib/runner.js:334:5)

  2) ReplaceSource
       should replace multiple items correctly:
     TypeError: (intermediate value).originalPositionFor is not a function
      at ReplaceSource._replacementToSourceNode (lib/ReplaceSource.js:156:54)
      at ReplaceSource.<anonymous> (lib/ReplaceSource.js:83:37)
      at Array.forEach (<anonymous>)
      at ReplaceSource.node (lib/ReplaceSource.js:76:21)
      at ReplaceSource.proto.sourceAndMap (lib/SourceAndMapMixin.js:30:18)
      at Context.<anonymous> (test/ReplaceSource.js:61:26)
      at callFn (/usr/lib/nodejs/mocha/lib/runnable.js:354:21)
      at Test.Runnable.run (/usr/lib/nodejs/mocha/lib/runnable.js:346:7)
      at Runner.runTest (/usr/lib/nodejs/mocha/lib/runner.js:442:10)
      at /usr/lib/nodejs/mocha/lib/runner.js:560:12
      at next (/usr/lib/nodejs/mocha/lib/runner.js:356:14)
      at /usr/lib/nodejs/mocha/lib/runner.js:366:7
      at next (/usr/lib/nodejs/mocha/lib/runner.js:290:14)
      at Immediate.<anonymous> (/usr/lib/nodejs/mocha/lib/runner.js:334:5)

@alexander-akait
Copy link
Member

@pravi right now impossibly, look https://github.com/mozilla/source-map/blob/master/CHANGELOG.md#070

Breaking change: Drop support for Node < 8. If you want to support older versions of node, please use v0.6 or below.

@andreicristianpetcu
Copy link

andreicristianpetcu commented Mar 4, 2018

Relevant downstream issue firefox-devtools/debugger#5598

@lencioni
Copy link

Do you think webpack-sources could be updated in a way that supports both the old and new versions of source-map? That would allow folks running Node 8+ to get the performance improvements without hurting folks on older Node. Thoughts?

@alexander-akait
Copy link
Member

@lencioni we work on this mozilla/source-map#326 (comment), can't do here, it should be solved in source-map package 😞

@lencioni
Copy link

lencioni commented Jul 9, 2018

@evilebottnawi I'm not sure I understand why this can't be resolved here, can you explain in a little more detail?

As far as the API is concerned, it seems that the main change is that source-map is now async instead of synchronous. Could we make this work by doing the following?

  1. Change webpack-sources to be asynchronous
  2. In webpack-sources, detect which version of source-map it has available and conditionally use the async one when that's the one that is installed, or fall back to the older synchronous one.
  3. Modify the source-map dependency range to allow 0.7+

This would give consumers the option to update to a newer version of source-map, if they are on a new enough version of Node, or continue using the much slower 0.6 if they are on an older version of Node.

Or, is there something else missing that I don't understand?

@mlavina
Copy link

mlavina commented Aug 7, 2018

Modify the source-map dependency range to allow 0.7+

I think this is the issue here because something like yarn will fail to install if there is an engine mismatch. If someone is on Node 6 trying to install this package which is using sourcemap 0.7 it will fail.

An option I have been thinking is either

  1. Have it as an optionalDependency and do some installing on runtime if the install failed
  2. Similar just do some kind of postinstall conditional install of source-map something like
"postinstall":  sh -c \"if [ node -p -e 'process.version > 8'  ] then; yarn install source-map@^0.7.1 else yarn install source-map@^0.6.1 fi\"
  1. Make source-map a Peer Dependency, but this would be a simple change, but technically a breaking change. Then at runtime check the version with something like
require('source-map/package.json').version

and have conditional code that way

@evilebottnawi I think this is related to our discussion on the uglify-js plugin webpack-contrib/uglifyjs-webpack-plugin#248
because i think a solution here will get translated to a solution there.

What are your thoughts on those 3 ideas?

@alexander-akait
Copy link
Member

alexander-akait commented Aug 7, 2018

@mlavina i think 2 point is looks good, let's do it first in plugin, example uglify/terser and then implement here, feel free to PR. Also some people use npm, better create install.js script and put logic here

@hedgepigdaniel
Copy link

hedgepigdaniel commented Sep 6, 2018

What about having an intermediate package - e.g. webpack-sources-source-map

It can have a peer dependency on source-map@^0.6.0 and an optional dependency on source-map@^0.7.0 (is that allowed...?). At runtime when it imports source-map it would end up with 0.7.0 only if it installed successfully. It could wrap both of them, and convert the synchronous API of 0.6.x into the asynchronous API of 0.7.x. That way webpack-sources can upgrade to the new API by using webpack-sources-source-map instead of source-map and still work on old node versions.

@mlavina
Copy link

mlavina commented Sep 22, 2018

It can have a peer dependency on source-map@^0.6.0 and an optional dependency on source-map@^0.7.0 (is that allowed...?)

Yeah that's the part that we need to figure out

At this point Node 6 is almost out of maintenance mode in April 2019 and I think at that point we can probably justify the breaking change to drop Node 6 support. If that's the case I am somewhat more inclined to wait then submit my proposed changes.

@marcins
Copy link

marcins commented Jul 15, 2019

What's the status of updating source-map in webpack-sources now that Node 6 is out of LTS and no longer supported? I believe this was the main blocker in the past.

FYI @TheLarkInn - the performance fix in mozilla/source-map#308 which never made it to the 0.6.x branch, but only to the new async/WASM 0.7.x branch of source-map appears to significantly improve performance for production (minified) builds with source maps .

As mentioned in mozilla/source-map#370 (comment) I applied the perf fix via patch-package and found a 70% decrease in our full Webpack 3 production build with sourcemaps (from 35 -> 12 minutes). I applied this to our smaller Webpack 4 build as well and saw an improvement there too, albeit smaller, from ~9 min to ~6.5 min. (that's still ~25% faster).

I will be trying upgrading our main build to Webpack 4 later this week, which we previously couldn't do due to a combo of perf & memory pressure which I now suspect was due to this issue.

Also, I notice that even the Webpack 5 branch is still using source-map 0.6.1 via the 2.0 beta of webpack-sources (at least from my reading of the dependencies).

@alexander-akait
Copy link
Member

alexander-akait commented Jul 16, 2019

In todo for webpack@5

@sokra
Copy link
Member

sokra commented Oct 22, 2019

Sorry, but the source-map@0.7 version is nearly impossible to use without completely changing our API to Promises.

In my opinion using WASM is not really needed for good performance in source-map. Or at least it's not worth the trade-off of additional complexity (async, managing wasm buffer initialization).

I really thinking about creating my own version of source-map without that stuff.

@alexander-akait
Copy link
Member

alexander-akait commented Oct 22, 2019

I think will be great to look on benchmarks, maybe we can open an issue about sync ability for source-map, terser maintainers need sync version too


Issue: mozilla/source-map#331

@marcins
Copy link

marcins commented Oct 22, 2019

To be honest I'm less concerned about the WASM etc stuff that comes with the newer versions of source-map, but the fix of a fundamental performance bug that has serious implications for CPU and memory usage when generating source maps for large compilations. We couldn't upgrade to Webpack 4 without applying this patch.

Maybe it would be enough as a starting point to fork source-map 0.6.x, and apply the perf fix, and use that forked version in the Webpack ecosystem? Ideally Mozilla would merge the back-port PR and release a new 0.6.x release, but if it hasn't happened by now (almost two years) it seems unlikely to happen.

@noppa
Copy link

noppa commented Jul 18, 2021

I just noticed 7rulnik/source-map-sync/pull/1 (source-map-sync on npm), which modifies source-map 0.8 to expose a synchronous API, by initializing the wasm module with new WebAssembly.Module(buffer) instead of WebAssembly.instantiate.

The downside of this approach is that it won't work in browsers, since they apparently have a 4kb size limit for the synchronous initialization API. I know there are some projects that use Webpack in the browser too, but perhaps it could be configured somehow that on Node, Webpack would use the source-map-sync library or something else like it.

I tried it on our project with yarn resolutions

  "resolutions": {
    "**/source-map": "npm:source-map-sync@0.8.0-beta.2"
  }

and it cut about 15-20% of our total production build time.

@7rulnik
Copy link

7rulnik commented Jul 18, 2021

Yep, but I prefer source-map-js

@sokra
Copy link
Member

sokra commented Jul 27, 2021

Major version 3 of webpack-sources is now super fast, and dependency-free. (not yet fully bug-free, but I'm working on that).

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

No branches or pull requests

10 participants