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

Update source-map -> 0.8.0-beta.0 for compatibility with global fetch #1164

Merged
merged 1 commit into from Apr 4, 2022

Conversation

robhogan
Copy link
Contributor

@robhogan robhogan commented Mar 14, 2022

source-map@0.7.3 has an issue with buggy identification of node vs browser environment (it uses typeof fetch === 'function' to spot browsers) - See mozilla/source-map#349, mozilla/source-map#432 and fixes mozilla/source-map#350, mozilla/source-map#363.

This is an issue for React (Native) environments where fetch is often polyfilled, eg for testing or SSR. It's one of the last issues holding us back from making terser the default minifier for https://github.com/facebook/metro.

Importantly, this is also probably going to be an issue for all NodeJS users soon, as NodeJS moves forward with a native global fetch: nodejs/node#41749

Unfortunately, the only source-map release since that bug was fixed is 0.8.0-beta.0, dated Nov 2018.

Would you consider a dependency on a "prerelease", or is this a non-starter?

Copy link
Collaborator

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

This is due to SourceMapConsumer using WASM, which uses fetch or fs.readFile to get the WASM bytes to instantiate. We could instead switch to @jridgewell/trace-mapping which provides a near drop-in API (and is faster, less memory intensive, and doesn't depend on WASM).

@jridgewell jridgewell added the dependencies Pull requests that update a dependency file label Mar 14, 2022
@fabiosantoscode
Copy link
Collaborator

🤔 this sounds tempting! The memory usage of source-map is definitely a problem. Though Terser also needs to handle the production of source maps. This library could help https://www.npmjs.com/package/vlq#-buffer

@robhogan
Copy link
Contributor Author

robhogan commented Mar 15, 2022

It is only the SourceMapConsumer part of source-map that's problematic though - while it'd be great to replace the source-map dependency entirely, just using something else for consumption would address the particular issue that motivated this PR.

(Specifically: source-map@0.7.3 will still incorrectly detect the environment on require('../lib/read-wasm'), but the issue doesn't manifest until readWasm() is called (and throws), which is only on instantiation of SourceMapConsumer here)

@jridgewell
Copy link
Collaborator

I tried creating a PR for @jridgewell/trace-mapping, but I see that there's a test for sections sourcemaps, which it doesn't currently support (I don't know of any build tool that produces it). I can try adding it later this week, but we can merge this PR for now.

@robhogan robhogan changed the title [RFC] Update source-map -> 0.8.0-beta.0 Update source-map -> 0.8.0-beta.0 for compatibility with global fetch Mar 21, 2022
@robhogan
Copy link
Contributor Author

robhogan commented Apr 4, 2022

@jridgewell - did you get anywhere with adding sections support? Otherwise, could we go with this PR as a mitigation?

Would love to move towards terser by default for React Native 👍

@jridgewell
Copy link
Collaborator

jridgewell commented Apr 4, 2022

I did not yet. I'm happy with using 0.8.0-beta for now, and I'll try updating to my package at a later time. /cc @fabiosantoscode

Implemented as AnyMap in v0.3.8. See #1181 for PR to switch.

@fabiosantoscode
Copy link
Collaborator

Sounds good to me!

@fabiosantoscode fabiosantoscode merged commit 7c98c9c into terser:master Apr 4, 2022
@fabiosantoscode
Copy link
Collaborator

Thanks for coming up with this @robhogan !

@marjorg
Copy link

marjorg commented Apr 20, 2022

This also closes #1030, who also deserves some credit for suggesting the fix ages ago :)

facebook-github-bot pushed a commit to facebook/metro that referenced this pull request May 30, 2022
Summary:
This update includes the removal of a direct dependency on `source-map@0.7.x`, in particular the problematic (especially for React Native) `fetch`-based browser/node environment detection described in terser/terser#1164.

This was the last item blocking us from deprecating and replacing `metro-minify-uglify`, which uses the long-depreceted [`uglify-es`](https://www.npmjs.com/package/uglify-es).

### Follow-up
I'll follow up with adding `metro-minify-terser` to `metro`'s dependencies and communicating the deprecation of `metro-minify-uglify` in OSS. Then we'll be able to switch the default and remove `metro-minify-uglify` from the deps in a release cycle or two.

Reviewed By: motiz88

Differential Revision: D36098090

fbshipit-source-id: 415431f17e8048bcd1eccd80c3faa6bbc2283cfb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants