-
Notifications
You must be signed in to change notification settings - Fork 79
fix(shell-api,shell-bson): increase runtime independence #2544
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
Conversation
Make the `shell-bson` and `shell-api` packages more runtime-independent (as they are supposed to be) by using `util` and `crypto` as progressive enhancement enablers rather than required packages. This specifically makes `shell-bson` fully usable in a bare-bones JS environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR reduces runtime dependencies for the shell-bson
and shell-api
packages by making util
and crypto
modules optional progressive enhancements rather than required imports. This allows these packages to work in bare-bones JavaScript environments.
Key changes include:
- Replacing direct crypto usage with BSON library UUID generation
- Implementing lazy loading of util.inspect with fallback functionality
- Converting promisify usage to native Promise constructor
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/shell-bson/src/shell-bson.ts | Replaces crypto-based UUID generation with bson.UUID() |
packages/shell-bson/src/printable-bson.ts | Adds lazy loading for util.inspect with fallback for typed array inspection |
packages/shell-bson/src/bson-export.ts | Adds UUID type imports and exports |
packages/shell-api/src/shell-api.ts | Replaces promisify with native Promise constructor in sleep method |
packages/shell-api/src/runtime-independence.spec.ts | Updates test to remove crypto and util from allowed Node builtins |
packages/shell-api/src/helpers.ts | Implements lazy loading for crypto and util.inspect with appropriate fallbacks |
packages/service-provider-node-driver/src/node-driver-service-provider.ts | Adds UUID to BSON library exports |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, although it does feel like ideally these should be pluggable so someone could still make something like inspect work the way they want if they care about it. Rather than it be either hardcoded as the common-js-imported module OR the hardcoded fallback.
@lerouxb Yeah, absolutely makes sense. For the crypto bit I've done this here, don't actually use it but you're right that it should be pluggable. For the |
Make the
shell-bson
andshell-api
packages more runtime-independent (as they are supposed to be) by usingutil
andcrypto
as progressive enhancement enablers rather than required packages.This specifically makes
shell-bson
fully usable in a bare-bones JS environment.