chore(shell-bson): split shell BSON logic into separate package #2537
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors MongoDB Shell's BSON handling by creating a new @mongosh/shell-bson package that separates BSON logic from service provider core, providing better architectural separation and making shell BSON constructors available for external projects.
Key Changes
- Creates new
@mongosh/shell-bsonpackage with generic, configurable BSON constructors - Moves BSON print handling and shell-specific extensions out of
service-provider-core - Updates dependencies across packages to use the new BSON package directly
Reviewed Changes
Copilot reviewed 70 out of 71 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/shell-bson/ | New package containing shell BSON logic, helpers, and printable BSON functionality |
| packages/shell-api/src/shell-instance-state.ts | Updated to construct shell BSON with help and metadata assignment |
| packages/service-provider-core/ | Removed BSON export and printable BSON logic, now depends on shell-bson |
| Various test and implementation files | Updated imports to use bson directly or @mongosh/shell-bson |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This is admittedly a large refactor, but one that I would believe is generally worth it: - Moving the BSON print handling out of `service-provider-core` makes sense, as `service-provider-core` is generally intended to be a library providing an *interface*, not partial implementations. - Moving the BSON classes out of `shell-api` is less architecturally important, but it does provide a little bit of additional encapsulation. It does, for example, force us to separate mongosh-specific `.help` property from core BSON constructor logic. - A number of files in other packages were using `@mongosh/service-provider-core` only as a convenient way of getting the current `bson` package, which works but is really more of a hack and there's no good reason not to just use the `bson` package that the driver team ships directly. This is also convenient on a broader basis, though, because it makes mongosh's BSON constructors generally available for other projects with similar needs to use. In Compass, we are avoiding this by using a Babel-based "manual JavaScript evaluator", but not all projects have the same technical requirements as Compass, and some may prefer the flexibility of a full-fledged JS execution environment like mongosh does.
addaleax
force-pushed
the
shell-bson-separate-package
branch
from
90b7568 to
1d526cc
Compare
There was a problem hiding this comment.
This file was moved, but the modifications were a little too extensive for the review display to follow
| ) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Extracted into a separate file
packages/shell-bson/README.md
Outdated
There was a problem hiding this comment.
Probably not a bad idea to start a review by looking at this file and trying the example code here 🙂
| } | ||
|
|
||
| // NB: Duplicate of the one in packages/shell-api/src/helpers.ts | ||
| export function assertArgsDefinedType( |
There was a problem hiding this comment.
^ Worth calling out the duplication here, I think
| describe('Shell BSON', function () { | ||
| let shellBson: any; | ||
| let shellBson: ShellBson<BSON, TestHelp>; |
There was a problem hiding this comment.
A lot of the changes in this file are because I naively thought this doesn't need to be any, it's quite noisy and introduces as any calls in other places so I honestly also wouldn't mind going back to the previous variant
There was a problem hiding this comment.
either is fine with me, since this is slightly more typed we can just stick with this 🙂
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
addaleax
added
the
no-title-validation
| } { | ||
| function fnCtor(...args: any[]) { | ||
| if (new.target) { | ||
| return Reflect.construct(ClassCtor, args, new.target); |
| describe('Shell BSON', function () { | ||
| let shellBson: any; | ||
| let shellBson: ShellBson<BSON, TestHelp>; |
There was a problem hiding this comment.
either is fine with me, since this is slightly more typed we can just stick with this 🙂
| assignMetadata?.(bson[className].prototype, { | ||
| help: help, | ||
| // Symbol is deprecated | ||
| ...(className === 'BSONSymbol' |
There was a problem hiding this comment.
not needed for this PR but makes me think for future if we should aim to have a single point of reference for different types of deprectations across mongosh. I know we're also ideally looking for this information to come from driver/bson(?) in the future so it'd probably make transition easier.
There was a problem hiding this comment.
Yeah, it would be awesome to have something standardized ... might require a bit of brainstorming together with the driver team about the approach 🙂
This is admittedly a large refactor, but one that I would believe is generally worth it:
service-provider-coremakes sense, asservice-provider-coreis generally intended to be a library providing an interface, not partial implementations.shell-apiis less architecturally important, but it does provide a little bit of additional encapsulation. It does, for example, force us to separate mongosh-specific.helpproperty from core BSON constructor logic.@mongosh/service-provider-coreonly as a convenient way of getting the currentbsonpackage, which works but is really more of a hack and there's no good reason not to just use thebsonpackage that the driver team ships directly.This is also convenient on a broader basis, though, because it makes mongosh's BSON constructors generally available for other projects with similar needs to use. In Compass, we are avoiding this by using a Babel-based "manual JavaScript evaluator", but not all projects have the same technical requirements as Compass, and some may prefer the flexibility of a full-fledged JS execution environment like mongosh does.