Repository navigation
Global fetch breaks source-map #42638
Description
Version
v17.8.0
Platform
Darwin Simens-MacBook-Pro.local 21.4.0 Darwin Kernel Version 21.4.0: Fri Mar 18 00:45:05 PDT 2022; root:xnu-8020.101.4~15/RELEASE_X86_64 x86_64
Subsystem
No response
What steps will reproduce the bug?
npm init -y && npm install --save source-map (this gives 0.7.3 at the time of writing). source-map has more than 160M downloads a week, although the broken v7 has a mere 28M since it's async only. See https://www.runpkg.com/?source-map@0.7.3/lib/read-wasm.js#1 for where it breaks.
// file.mjs
import {SourceMapConsumer} from 'source-map'
await new SourceMapConsumer({version: '3', sources: [], mappings: []})$ node file.mjs
$ echo $?
0
$ node --experimental-fetch file.mjs
(node:21808) ExperimentalWarning: Fetch is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
/Users/simen/repos/source-map-boom/node_modules/source-map/lib/read-wasm.js:8
throw new Error("You must provide the URL of lib/mappings.wasm by calling " +
^
Error: You must provide the URL of lib/mappings.wasm by calling SourceMapConsumer.initialize({ 'lib/mappings.wasm': ... }) before using SourceMapConsumer
at readWasm (/Users/simen/repos/source-map-boom/node_modules/source-map/lib/read-wasm.js:8:13)
at wasm (/Users/simen/repos/source-map-boom/node_modules/source-map/lib/wasm.js:25:16)
at /Users/simen/repos/source-map-boom/node_modules/source-map/lib/source-map-consumer.js:264:14
at async file:///Users/simen/repos/source-map-boom/file.mjs:3:1
Node.js v17.8.0
$ echo $?
1How often does it reproduce? Is there a required condition?
Always.
What is the expected behavior?
It should not break since the code is not running in a browser environment.
What do you see instead?
If fetch is available as a global, source-map thinks it's in a browser setting, and breaks.
Additional information
I know this isn't Node's fault, but since source-map seems unmaintained, I hope maybe Node as a project can reach out the the old maintainers (or Mozilla?) so this is fixed.
Once v18 is stable (where fetch is a global by default), this'll break way more people. v8-to-istanbul, used by both C8 and Jest (to provide code coverage reports when using native V8 coverage) is affected by this, and probably lots of other projects.
Activity
aduh95 commentedon Apr 7, 2022
Is there an issue open for this on their repo?
SimenB commentedon Apr 7, 2022
mozilla/source-map#349. It's apparently fixed in the beta releases
SimenB commentedon Apr 7, 2022
Based on terser/terser#1164 (review)
v8-to-istanbulmight benefit from migrating, but there's still 28M weekly downloads of the broken version (jest has maybe half of those? unsure how npm counts)aduh95 commentedon Apr 7, 2022
FWIW there's the
--no-experimental-fetchCLI flag that can be used as a workaround.Pinging @nodejs/npm in case they know how this could be mitigated.
SimenB commentedon Apr 7, 2022
Ah, nice! I can add that flag to Jest's tests at least
targos commentedon Apr 10, 2022
Is there still something for Node.js to do?
SimenB commentedon Apr 10, 2022
As a project it would be nice if node could try to get in touch with the maintainers so they make a new stable release before v18 is released. But code wise I doubt there's much to be done (beyond not unflagging
fetchwhich I assume (and hope) is a non-starter).ljharb commentedon Apr 10, 2022
Node could also use a different name than fetch, or put it somewhere that’s not a global.
SimenB commentedon Apr 20, 2022
C8 migrated away in a patch, and Jest in an alpha release. Still, that's only about half of the downloads of the broken version
devsnek commentedon Apr 20, 2022
are these packages in our npm canary tests? obviously breaking changes are expected in major versions but it makes a good way to notify at least.
SimenB commentedon Apr 21, 2022
Jest is (which is how I was made aware): nodejs/citgm#894. I don't think
c8orsource-mapis.ljharb commentedon Apr 21, 2022
Heads up that this continues to be broken for source-map in node 18, which is also breaking
tsup, which is used by https://github.com/coinbase/cbpay-js/runs/6116903030?check_suite_focus=true. Given that mozilla/source-map#349 was last updated in 2019, there's not a strong implication that a fix will be published anytime soon.4 remaining items
ljharb commentedon May 11, 2022
@mcollina yes; see #42638 (comment) - jest may have worked around it but all the other source-map dependents likely haven't.
mcollina commentedon May 11, 2022
Is there any fix we can do on the Node.js side?
ljharb commentedon May 11, 2022
@mcollina the only fix i can think of is no longer having a global named
fetch, since that's what this widely-deployed package interprets to mean "it's a browser" - either a global under another name, or a non-global, would ofc fix it.dnalborczyk commentedon May 11, 2022
please leave it a global named
fetch.node.jsis not a browser and the source files are accessible and can be forked. I would not let an unmaintained package dictate the future offetchon node.js.one can also
fix itby trying to contact someone at mozilla or just use a fork.ljharb commentedon May 11, 2022
@dnalborczyk it's not a browser which is why it shouldn't have a
fetchglobal :-)"use a fork" doesn't work because it's a widely used transitive dependency. Certainly fixing source-map itself is an option, but that's not under node's control.
dnalborczyk commentedon May 12, 2022
I'm sorry, not quite sure if I'm following your argument 😄 if you are not joking, your argument could be applied to any of the supported web api in node.js. why shouldn't it be supported just because it's not a browser?
well, node.js 18 was released very recently. most production code use LTS versions. so there's still some time to correct the issue in any transitive packages, be it by a mozilla fix or a fork, until v18 reaches LTS.
changing the global name now would be a BREAKING change in itself.
I was thinking about this a bit. there's another possible solution by providing the
fetchAPI to the user with an explicitfetchexport, (e.g.import fetch, { Headers, Request } from 'node:fetch', in addition to the global.if an app wants to use
fetchas well as thesource-mapmodule (or any transitive dependencies using it) a developer could turn off the global module with the provided flag and import fetch explicitly if needed.ljharb commentedon May 12, 2022
@dnalborczyk the api is experimental so it wouldn’t require a semver-major to remove, but i understand that folks would be unlikely to agree to that.
My argument is that a web API should only exist in node - at all, or as a global - if it can 100% match the semantics. fetch can’t. Obviously node folks rejected that argument, or it wouldn’t have landed in node 18, but my opinion remains.
SimenB commentedon May 12, 2022
Note that the alpha of
source-mapis fixed, so if somebody ™️ could get a hold of one of the maintainers, just promoting that to stable might help (or even backporting the fix to v0.7). At least that should let the fix trickle down over time. Main issue now (imo) is thatsource-map@latestis broken.mcollina commentedon Jun 1, 2022
@eemeli do you think you can ship an updated version of source-map?
eemeli commentedon Jun 1, 2022
I'll see what I can do.
eemeli commentedon Jun 4, 2022
source-map@0.7.4is now published aslateston npm, with no breaking changes to the previous version.mcollina commentedon Jun 5, 2022
Thanks @eemeli! I think we can close this now.