Skip to content
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

fix(NODE-6074): Avoiding top-level awaits in bson by introducing "node" bundles #669

Closed
wants to merge 8 commits into from

Conversation

kraenhansen
Copy link
Member

@kraenhansen kraenhansen commented Apr 11, 2024

Description

What is changing?

I suggest introducing two node specific bundles, which performs the import of node:crypto.

The "realm" SDK uses a similar pattern of platform specific entrypoints declared via separate export conditions and TS project references.

Is there new documentation needed for these changes?

What is the motivation for this change?

See https://jira.mongodb.org/browse/NODE-6074

Release Highlight

Fill in title or leave empty for no highlight

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@kraenhansen kraenhansen self-assigned this Apr 11, 2024
@@ -107,7 +109,7 @@
"check:granular-bench": "npm run build:bench && node ./test/bench/etc/run_granular_benchmarks.js",
"check:spec-bench": "npm run build:bench && node ./test/bench/lib/spec/bsonBench.js",
"build:bench": "cd test/bench && npx tsc",
"build:ts": "node ./node_modules/typescript/bin/tsc",
"build:ts": "node ./node_modules/typescript/bin/tsc --build --force",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding --build because the project now relies on TS project references.
Adding --force because the composite: true entails incremental: true and subsequent runs of build:dts would fail because the clean_definition_files deletes files produced by tsc and because of the incremental: true it TSC doesn't check the existence of cached files. The --force opts out of incremental builds.

@kraenhansen kraenhansen marked this pull request as draft April 11, 2024 13:05
@kraenhansen
Copy link
Member Author

It seems there are some type issues with the tests that I have to sort out, but I'll have to end for today.
I'll put this out of draft when I'm ready with those changes 👍

Copy link
Member

@elle-j elle-j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would alleviate things for us on the Realm JS side 🙏

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We plan to meet about this so we can dive into more detail over a call but this change is a reversal of prior design decision to unify the javascript that is run in our supported environments.

It is unfortunate that a language feature like top-level await still causes a hassle with bundling but we and others have dealt with this in the past by enabling the flags needed to import such syntax or forced resolution to use the commonjs module.

@kraenhansen
Copy link
Member Author

Okay. We'll revisit our configurations and try to make it work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants