Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The production lazy-loading and consumer-bundling paths central to this fix lack integration regression coverage.
1 open issue
What changed in this PR
This PR replaces web-worker with native worker adapters and inlined worker source so shell-bson-parser can work in bundled consumers without resolving worker scripts by file path.
Changes:
- Generate a lazily loaded worker-source module and simplify the CommonJS build.
- Use blob-backed browser workers and native Node.js worker threads.
- Update lifecycle tests, documentation, and dependencies.
| File | Description |
|---|---|
| packages/shell-bson-parser/webpack.worker.config.cjs | Generate the inlined worker-source module. |
| packages/shell-bson-parser/webpack.cjs.config.cjs | Remove shim workarounds and externalize worker-source loading. |
| packages/shell-bson-parser/test/fixtures/slow-worker.js | Use native Node.js worker messaging. |
| packages/shell-bson-parser/src/worker.ts | Handle requests through Node.js parentPort. |
| packages/shell-bson-parser/src/worker-types.ts | Define shared worker handles and callbacks. |
| packages/shell-bson-parser/src/worker-source.ts | Add a placeholder for generated worker source. |
| packages/shell-bson-parser/src/worker-client.ts | Load source lazily and select native worker adapters. |
| packages/shell-bson-parser/src/node-worker.ts | Implement the Node.js worker adapter. |
| packages/shell-bson-parser/src/index.spec.ts | Update source injection and worker lifecycle coverage. |
| packages/shell-bson-parser/src/browser-worker.ts | Implement blob-backed browser workers and cleanup. |
| packages/shell-bson-parser/README.md | Document native workers and bundled source loading. |
| packages/shell-bson-parser/package.json | Remove the web-worker dependency. |
| package-lock.json | Remove corresponding dependency lock entries. |
Review effort: Balanced
| new Blob([code], { type: 'text/javascript' }), | ||
| ); | ||
| return blobUrl; | ||
| const source = await import('./worker-source.js'); |
nbbeeken
left a comment
nbbeeken
left a comment
There was a problem hiding this comment.
Why doesn't the web-worker package still work if we give it the blob input? I'm thinking the logic that abstracts node vs web is still useful, just how we load the src needs fixing, no?
| ): WorkerHandle { | ||
| // Compass Web injects whatwg-url for bare URL references; it lacks blob APIs. | ||
| // Use the native constructor for blob creation and revocation. | ||
| const blobUrl = globalThis.URL.createObjectURL( |
There was a problem hiding this comment.
| const blobUrl = globalThis.URL.createObjectURL( | |
| const blobUrl = URL.createObjectURL( |
Is there a reason to be so explicit about the globalThis? same for Worker?
There was a problem hiding this comment.
Okay, although i closed this PR, I still have it in the code already. This is for compass-web. We resolve url to whatwg-url and that does not have blob-url methods. This breaks for compass-web and i had to go this route for that.
What would be the better alternate here?
| // Compass Web injects whatwg-url for bare URL references; it lacks blob APIs. | ||
| // Use the native constructor for blob creation and revocation. | ||
| const blobUrl = globalThis.URL.createObjectURL( | ||
| new Blob([source], { type: 'text/javascript' }), |
There was a problem hiding this comment.
I recently learned that this is allowed even when you're running node with eval disabled... neat! but concerning? I don't know... nothing to action here lol
| throw new Error( | ||
| '@mongodb-js/shell-bson-parser needs a global Worker or process.getBuiltinModule', | ||
| ); |
There was a problem hiding this comment.
Well in this case the global wouldn't help this code work right?
Description
parsebreaks once a consumer bundles this package, because it finds its worker by file path. The CJS build turnsimport.meta.urlintorequire("node:url"),require("node:path")and__dirname:toJSStringis imported. We hit this in the VS Code extension's webview.__dirnamepoints into the consumer's bundle, soworker.jsisn't found. web-worker also starts its thread withnew Worker(__filename), which is the consumer's bundle.This PR makes the worker self-contained, so it no longer depends on where files end up after bundling:
The worker is inlined as a string. A build step writes the bundled
dist/worker.jsintodist/worker-source.js. The client loads it with a lazyimport(), so consumers that only usetoJSStringdon't load it.Browsers start the worker from a
blob:URL. The client no longer needs the fetch path or a served worker.js.Node starts it with
worker_threads.worker_threadsis loaded viaprocess.getBuiltinModule, which bundlers don't rewrite or polyfill. The worker answers onparentPortitself, so no shim is needed.The
web-workerdependency is removed, along with the CJS build's DefinePlugin and web-worker workarounds.Open Questions
Checklist