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

node v18.0.0-nightly20220222c071bd581a's fetch() breaks current sourcemap support #2589

Closed
trentm opened this issue Feb 28, 2022 · 4 comments · Fixed by #2596
Closed

node v18.0.0-nightly20220222c071bd581a's fetch() breaks current sourcemap support #2589

trentm opened this issue Feb 28, 2022 · 4 comments · Fixed by #2596
Assignees
Labels
agent-nodejs Make available for APM Agents project planning.

Comments

@trentm
Copy link
Member

trentm commented Feb 28, 2022

The node v18.0.0-nightly20220222c071bd581a release from a few days ago started breaking nightly tests (https://apm-ci.elastic.co/job/apm-agent-nodejs/job/apm-agent-nodejs-mbp/job/main/53/):

...
[2022-02-23T08:05:13.285Z] node_tests_1     | running test: cd . && node --unhandled-rejections=strict test/sourcemaps/index.test.js > test_output/test-sourcemaps-index.test.js.tap 2&>1
[2022-02-23T08:05:14.225Z] node_tests_1     | 
[2022-02-23T08:05:14.225Z] node_tests_1     | TAP version 13
[2022-02-23T08:05:14.225Z] node_tests_1     | # source map inlined
[2022-02-23T08:05:14.225Z] node_tests_1     | ok 1 should be strictly equal
[2022-02-23T08:05:14.225Z] node_tests_1     | ok 2 should be strictly equal
[2022-02-23T08:05:14.225Z] node_tests_1     | not ok 3 should be strictly equal
[2022-02-23T08:05:14.225Z] node_tests_1     |   ---
[2022-02-23T08:05:14.225Z] node_tests_1     |     operator: equal
[2022-02-23T08:05:14.225Z] node_tests_1     |     expected: |-
[2022-02-23T08:05:14.225Z] node_tests_1     |       'generateError (test/sourcemaps/fixtures/src/error.js)'
[2022-02-23T08:05:14.226Z] node_tests_1     |     actual: |-
[2022-02-23T08:05:14.226Z] node_tests_1     |       'generateError (test/sourcemaps/fixtures/lib/error-inline.js)'
[2022-02-23T08:05:14.226Z] node_tests_1     |     at: assertSourceFound (/app/test/sourcemaps/index.test.js:105:5)
[2022-02-23T08:05:14.226Z] node_tests_1     |     stack: |-
[2022-02-23T08:05:14.226Z] node_tests_1     |       Error: should be strictly equal
[2022-02-23T08:05:14.226Z] node_tests_1     |           at Test.assert [as _assert] (/app/node_modules/tape/lib/test.js:314:54)
[2022-02-23T08:05:14.226Z] node_tests_1     |           at Test.bound [as _assert] (/app/node_modules/tape/lib/test.js:99:32)
[2022-02-23T08:05:14.226Z] node_tests_1     |           at Test.strictEqual (/app/node_modules/tape/lib/test.js:478:10)
[2022-02-23T08:05:14.226Z] node_tests_1     |           at Test.bound [as strictEqual] (/app/node_modules/tape/lib/test.js:99:32)
[2022-02-23T08:05:14.226Z] node_tests_1     |           at assertSourceFound (/app/test/sourcemaps/index.test.js:105:5)
[2022-02-23T08:05:14.226Z] node_tests_1     |           at Object.sendError (/app/test/sourcemaps/index.test.js:96:7)
[2022-02-23T08:05:14.226Z] node_tests_1     |           at filterAndSendError (/app/lib/agent.js:108:639)
[2022-02-23T08:05:14.226Z] node_tests_1     |           at finish (/app/lib/errors.js:72:617)
[2022-02-23T08:05:14.226Z] node_tests_1     |           at /app/lib/errors.js:66:158
[2022-02-23T08:05:14.226Z] node_tests_1     |           at finish (/app/lib/stacktraces.js:89:191)
[2022-02-23T08:05:14.226Z] node_tests_1     |           at /app/node_modules/after-all-results/index.js:20:25
[2022-02-23T08:05:14.226Z] node_tests_1     |           at processTicksAndRejections (node:internal/process/task_queues:77:11)
[2022-02-23T08:05:14.226Z] node_tests_1     |   ...
...

With verbose logging, the issue in the APM agent's usage of the source-map package is this:

[2022-02-28T18:48:02.800Z] DEBUG (elastic-apm-node): could not process file source map
    filename: "/Users/trentm/el/apm-agent-nodejs12/test/sourcemaps/fixtures/lib/error-inline.js"
    error: {
        "type": "Error",
        "message":
            Error parsing sourcemap for file "/Users/trentm/el/apm-agent-nodejs12/test/sourcemaps/fixtures/lib/error-inline.js":
            You must provide the URL of lib/mappings.wasm by calling SourceMapConsumer.initialize({ 'lib/mappings.wasm': ... }) before using SourceMapConsumer,
        "stack_trace":
            Error: Error parsing sourcemap for file "/Users/trentm/el/apm-agent-nodejs12/test/sourcemaps/fixtures/lib/error-inline.js":
            You must provide the URL of lib/mappings.wasm by calling SourceMapConsumer.initialize({ 'lib/mappings.wasm': ... }) before using SourceMapConsumer
                at readWasm (/Users/trentm/el/apm-agent-nodejs12/node_modules/source-map/lib/read-wasm.js:8:13)
                at wasm (/Users/trentm/el/apm-agent-nodejs12/node_modules/source-map/lib/wasm.js:25:16)
                at /Users/trentm/el/apm-agent-nodejs12/node_modules/source-map/lib/source-map-consumer.js:264:14
    }

This is due to:

  1. The current release version of source-map v0.7.3 uses if (typeof fetch === "function") to differentiate between browser and node.js environments: if (typeof fetch === "function") misidentifies node environment mozilla/source-map#349
  2. And because node#master recently enabled an implementation of fetch by default: lib: enable fetch by default nodejs/node#41811

The same issue (broken source-map support) happened with early builds of node v18 when using the --experimental-fetch CLI option. The issue can be worked around in recent node v18 nightly builds by using the --no-experimental-fetch CLI option.
The same issue happens if fetch() is polyfilled, e.g. as Next.js does (according to discussion at GoogleChrome/workbox#2712).

source-map hasn't had a release since May 2018, before mozilla/source-map#349 was fixed. There is a beta release that could be considered:

    "0.7.3": "2018-05-16T17:29:49.200Z",
    "0.8.0-beta.0": "2018-11-16T00:03:32.324Z"
@trentm trentm self-assigned this Feb 28, 2022
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Feb 28, 2022
@trentm
Copy link
Member Author

trentm commented Feb 28, 2022

There is a beta release that could be considered:

changes: mozilla/source-map@0.7.3...0.8.0-beta.0
changelog: https://github.com/mozilla/source-map/blob/master/CHANGELOG.md#080-beta0

@trentm
Copy link
Member Author

trentm commented Feb 28, 2022

I've opened rexxars/load-source-map#6 on the load-source-map repo that would fix this for us.

Options:

  1. Wait for a release with that PR and update. Meanwhile ignore "nightly" test failures until then, or skip the sourcemap tests with node v18 until then.
  2. Inline the load-source-map functionality and drop that dep. Then we can control the version of 'source-map' it uses.

@trentm
Copy link
Member Author

trentm commented Mar 4, 2022

Note to self to repro:

export PATH=$HOME/tmp/node-v18.0.0-nightly20220222c071bd581a-darwin-x64/bin:$PATH
node test/sourcemaps/index.test.js

@trentm
Copy link
Member Author

trentm commented Mar 4, 2022

Option 3: This spectacularly awful hack:

diff --git a/lib/stacktraces.js b/lib/stacktraces.js
index 3ea1b962..9f1540e9 100644
--- a/lib/stacktraces.js
+++ b/lib/stacktraces.js
@@ -14,7 +14,10 @@ const asyncCache = require('async-cache')
 const afterAllResults = require('after-all-results')
 const errorCallsites = require('error-callsites')
 const errorStackParser = require('error-stack-parser')
+const _fetchCache = global.fetch
+if (_fetchCache !== undefined) delete global.fetch
 const loadSourceMap = require('load-source-map')
+if (_fetchCache !== undefined) global.fetch = _fetchCache
 const LRU = require('lru-cache')

 const fileCache = asyncCache({

trentm added a commit that referenced this issue Mar 4, 2022
Recent node v18 turned on an implementation of `fetch()` by default.
The 'source-map@0.7.3' dep used for sourcemap handling incorrectly
uses the presence of `fetch` to decide it is in a browser env.

This change inlines the load-source-map dep so we can update it to
use the *beta* source-map@0.8.0-beta.0 which includes a fix.

Fixes: #2589
trentm added a commit that referenced this issue Mar 5, 2022
Recent node v18 turned on an implementation of `fetch()` by default.
The 'source-map@0.7.3' dep used for sourcemap handling incorrectly
uses the presence of `fetch` to decide it is in a browser env.

This change inlines the load-source-map dep so we can update it to
use the *beta* source-map@0.8.0-beta.0 which includes a fix.

Fixes: #2589
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant