fix: unwrap ESM-style process polyfills in internal streams (fixes #539) - #557
fix: unwrap ESM-style process polyfills in internal streams (fixes #539)#557smessie wants to merge 2 commits into
Conversation
Refs: nodejs#539 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: smessie <smessie@smessie.com>
|
Any chance we can have a look at this @mcollina ? |
|
ci looks red |
|
@mcollina This is because the CI still uses unsupported Node.js versions. I've pushed a commit that updates the CI to the latest LTS versions. |
|
Does this look good to you @mcollina or do you prefer another way to solve this CI problem? |
|
Any chance we can get this merged @mcollina ? |
| node-version: 12.x | ||
| - os: macos-latest | ||
| node-version: 14.x | ||
| node-version: [22.x, 24.x, 26.x] |
There was a problem hiding this comment.
Unfortunately, we still needs to test 12, 14, 16, 18, 20 and 21.
83eb7e9 to
047741a
Compare
playwright is declared as "^1.21.1", so a fresh CI install resolves to 1.62.0, which requires Node.js 20 or higher. The browsers workflow was still on Node.js 16, so "playwright install" aborted before any test ran. Only this workflow is bumped; node.yml and bundlers.yml keep testing the older Node.js versions.
047741a to
9379ad0
Compare
|
The failing CI is not related to this PR, yet I pushed a commit that solves it. I dropped my previous commit such that it is testing against the old Node.js versions again. Does this suffice, @mcollina? If not, please be specific about the path you want to take. I’ll then determine if I still have the time to investigate this, or if I’ll have to abandon this effort. |
|
Any word on this @mcollina ? |
Summary
Some browser bundlers/polyfill setups expose the
process/polyfill as an ES module with adefaultexport (i.e.{ __esModule: true, default: <process> }). The internal streams code doesconst process = require('process/')and then usesprocess.nextTick(...), which throws in that scenario because the module namespace object has nonextTick.This routes the internal
process/imports through a small shim that unwraps such ESM-style polyfills (returning the first candidate exposingnextTick) while leaving normal CommonJS polyfills untouched.Fixes #539.
This PR follows a similar approach as #543.
Changes
lib/internal/shims/process.js(generated fromsrc/) that unwraps an ESM-defaultprocess/polyfill, validating candidates via'nextTick' in candidate, and falls back to the module as-is otherwise.destroy,duplexify,end-of-stream,from,pipeline,readable,writable) through the shim instead ofrequire('process/')directly.Tests
test/ours/test-process-shim.jsverifying the shim resolves the real process object (withnextTick) when given an ESM-default-shaped polyfill, and returns a plain process-like object unchanged.npm test,npm run lint,npm run test:format, and the browserify bundle test all pass.Context
This fix is currently carried as a
patch-packagepatch in Comunica to make it work in the browser; upstreaming it here removes the need for the patch. See comunica/comunica#1724.