-
Notifications
You must be signed in to change notification settings - Fork 256
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): Removes top-level await in bson with separate node and browser ESM bundles #749
base: main
Are you sure you want to change the base?
Conversation
Pinging @nbbeeken for review 🙏 |
@lourd Hi there, thanks for bringing this to our attention! Neal is unavailable, but the team will re-triage this issue next week and get back to you with a reply as soon as we determine the best course of action. |
Hey @lourd, thanks for opening this PR. The current package structure was an intentional decision to avoid separate ESM bundles for Node and the browser because the current ESM module can be run in both. In our next major release, we plan to drop support for Node16 and Node18. Node20+ support global crypto by default, which will allow us to remove the import entirely (and the top-level await in the ESM module by extension). We'd really like to keep the existing bundle structure, if possible, until we can make this change. We tend to make our major releases in late summer (roughly August). We'd like to know more about the problems that this is causing for your users.
And finally - is the current experience acceptable for the next ~5ish months, knowing that a proper resolution to this issue is coming in [email protected]? |
Hey @baileympearson, for sure, thanks for the consideration and context!
At least anyone using Next.js and the bson library on the client. I suspect that it really encompasses anyone using React+Fast Refresh with Webpack, as it's not Next.js-specific. Users are affected whether they import
Fast Refresh is broken on the pages where
I tried targeting the CJS module as I saw suggested in your other issues first, by overriding the
It's not acceptable for us, hence going ahead and patching the package locally. What's the cost-benefit tradeoff of having separate ESM bundles for Node and the browser? It seems like a relatively trivial change. |
I'm also affected by this, so adding my view. I'm part of a team maintaining libraries for browsers and react-native, that have the bson package as a dependency, and are users are affected by this issue. For example, when using vite (which is quite common), users need to add the I've seen this also cause issues with Angular's build system, although I can't find the details now. Overall, there are generally workarounds, but this does cause pain for web users. |
Description
What is changing?
The bundling process has been changed to make two separate ESM bundles for browsers and Node.js respectively.
Is there new documentation needed for these changes?
No
What is the motivation for this change?
The main motivation for this change is that the top level async-await usage breaks hot refresh in Next.js when importing this library on the client, as documented in this issue vercel/next.js#61452 with a reproduction in this repo https://github.com/flo-dao/repro-fast-refresh.
The code does execute fine, it's specifically that the hot refresh behavior is broken. This is very problematic on applications that are very stateful and leverage hot refresh to have a quick iteration cycle. The current state is that when changing any module that imports
"bson"
it causes a full-page refresh, and you lose all of the state driving the UI you were developingThis has been raised before in this repo in #669 with a corresponding ticket that was closed without resolution https://jira.mongodb.org/browse/NODE-6074.
Release Highlight
Fill in title or leave empty for no highlight
Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description