Repository navigation
Conversation
e5d8ab7 to
a596dfa
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical API, compatibility, browser-runtime, and BSON-cloning issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 4
Open (6)
Removing CommonJS entry breaks CommonJS consumers · New Synchronous query-parser consumer breaks with async default API · New Duck-typed BSON detection corrupts valid documents · New Unguarded process access crashes browser parsing · New Concurrent worker initialization leaks duplicate workers · New README documents outdated synchronous parse API · New
What changed in this PR
Moves BSON parsing into a web worker while adding structured-clone support, an explicit synchronous API, and ESM/Webpack tooling.
Changes:
- Adds worker communication, lifecycle management, and BSON serialization.
- Makes default parsing asynchronous while retaining synchronous parsing.
- Updates tests, packaging, build configuration, and the MQL docs crawler.
| File | Review summary |
|---|---|
packages/shell-bson-parser/webpack.worker.config.cjs |
Bundles the worker target. |
packages/shell-bson-parser/tsconfig.json |
Enables NodeNext resolution. |
packages/shell-bson-parser/test/parse-test-cases.ts |
Updates ESM imports and test typing. |
packages/shell-bson-parser/src/worker.ts |
Handles worker parse requests. |
packages/shell-bson-parser/src/worker-types.ts |
Defines worker messages. |
packages/shell-bson-parser/src/worker-client.ts |
Critical: browser process access can throw. Moderate: concurrent initialization can leak workers, and termination can leave requests unresolved. |
packages/shell-bson-parser/src/structured-clone-bson.ts |
Critical: _bsontype duck typing can corrupt valid plain documents after cloning. |
packages/shell-bson-parser/src/structured-clone-bson.spec.ts |
Tests BSON round trips. |
packages/shell-bson-parser/src/stringify.spec.ts |
Updates ESM imports. |
packages/shell-bson-parser/src/parse.ts |
Updates internal ESM imports. |
packages/shell-bson-parser/src/parse.spec.ts |
Updates imports and Sinon usage. |
packages/shell-bson-parser/src/index.ts |
Critical: the async default API breaks synchronous query-parser usage. Nit: README examples remain synchronous. |
packages/shell-bson-parser/src/index.spec.ts |
Moderate: tests bypass the bundled worker path and do not cover the real worker artifact. |
packages/shell-bson-parser/src/eval.ts |
Updates an internal import. |
packages/shell-bson-parser/src/check.ts |
Updates internal imports. |
packages/shell-bson-parser/package.json |
Critical: removing the CommonJS condition breaks CommonJS consumers. Moderate: type: module breaks the existing CommonJS benchmark. |
packages/shell-bson-parser/.mocharc.cjs |
Adds shared Mocha configuration. |
packages/shell-bson-parser/.eslintrc.cjs |
Ignores the Webpack configuration. |
packages/mql-typescript/src/driverSchema/docs-crawler.ts |
Uses the synchronous parser API. |
package-lock.json |
Locks updated Webpack dependencies. |
configs/mocha-config-devtools/register/tsnode-register.js |
Adds NodeNext test-loader support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "type": "module", | ||
| "main": "dist/index.js", | ||
| "types": "./dist/index.d.ts", | ||
| "exports": { | ||
| "require": { | ||
| "types": "./dist/index.d.ts", | ||
| "default": "./dist/index.js" | ||
| }, | ||
| "import": { | ||
| "types": "./dist/index.d.ts", | ||
| "default": "./dist/.esm-wrapper.mjs" | ||
| } | ||
| "types": "./dist/index.d.ts", |
There was a problem hiding this comment.
Yes, this change in intentional. We deprecated mongodb-query-parser package and marked it as private in this PR (and also excluded it from tests/checks). As for mql-typescript, changed to dynamic imports
|
|
||
| export const parse = ( | ||
| ...args: Parameters<typeof parseSync> | ||
| ): Promise<ReturnType<typeof parseSync>> => callWorker(args); |
There was a problem hiding this comment.
Yes this is a breaking change and mongodb-query-parser is deprecated.
| function isBsonValue(value: object): value is { _bsontype: string } { | ||
| return typeof (value as { _bsontype?: unknown })._bsontype === 'string'; |
| } | ||
|
|
||
| async function getWorkerBlobUrl(): Promise<string> { | ||
| if (process.env.TEST_SKIP_WORKER_SCRIPT_FETCH) { |
| if (worker) { | ||
| return worker; | ||
| } | ||
|
|
||
| blobUrl = await getWorkerBlobUrl(); |
| export const parse = ( | ||
| ...args: Parameters<typeof parseSync> | ||
| ): Promise<ReturnType<typeof parseSync>> => callWorker(args); |
d0e8103 to
15408ac
Compare
| "name": "mongodb-query-parser", | ||
| "description": "Parse MongoDB queries", | ||
| "deprecated": "This package is deprecated. Use @mongodb-js/shell-bson-parser instead.", | ||
| "private": true, |
There was a problem hiding this comment.
Ignores from being published.
89987eb to
b87d456
Compare
esvm
left a comment
There was a problem hiding this comment.
just a small question but LGTM
fd1ff4b to
2d47a92
Compare
|
3cad193 (compass-components bump) brought many changes to lock file. |
Anemy
left a comment
There was a problem hiding this comment.
Changes are looking good, I left a few questions, not blockers.
0e2a4b5 to
1a74167
Compare
| const tag = | ||
| item._bsontype === 'Binary' && | ||
| (item as { sub_type?: unknown }).sub_type === 4 | ||
| Object.getPrototypeOf(item) === BSON.UUID.prototype |
There was a problem hiding this comment.
is this not just the same as doing instanceof?
| } | ||
| const proto = Reflect.getPrototypeOf(value); | ||
| return proto !== Object.prototype && proto !== null; | ||
| return KNOWN_BSON_PROTOTYPES.has(Object.getPrototypeOf(value)); |
There was a problem hiding this comment.
Couldn't we just rely on the value of the string here? and could we use the [bsonType] symbol that is exported from the BSON library? then its indisputable. The ambiguous case we have to watch out for is plain object with a _bsontype property, but that goes away with the symbol.
There was a problem hiding this comment.
Due to wide range of bson version we support, i'll keep using _bsonType as symbol is available from bson 7.0
1a74167 to
ebf38c5
Compare
ebf38c5 to
cf32098
Compare



In draft becauseI need to spend a bit more on (de)serialization (Neal is working on something very similar) and bundling.I need to figure out the sync -> async api change for mongodb-query-parser package. This changes happened in this PR and for monorepo, it resolves to the local export and not the one pinned in package.json file.Description
COMPASS-11127
Open Questions
Checklist
Stack created with GitHub Stacks CLI • Give Feedback 💬