mongodb
fix(NODE-6074): Removes top-level await in bson with separate node and browser ESM bundles
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 https://github.com/vercel/next.js/issues/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 developing
This has been raised before in this repo in https://github.com/mongodb/js-bson/pull/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
- [x] Ran
npm run check:lintscript - [x] Self-review completed using the steps outlined here
- [x] PR title follows the correct format:
type(NODE-xxxx)[!]: description - [x] Changes are covered by tests
- [x] New TODOs have a related JIRA ticket
@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.
- How many users seem to be impacted? It seems like a relatively narrow subset of users - users with data heavy client-side applications that also use BSON in the browser?
- What are the actual consequences of this issue? If I understand correctly, it seems like it just reloads extra modules and would require that users re-navigate their application to get to the place they were before the refresh.
- Is there truly no workaround here? @nbbeeken suggested perhaps targeting the CJS module and using a crypto polyfill might resolve the issue. Or something else?
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!
- How many users seem to be impacted?
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 bson directly or indirectly. I haven't tried with other bundlers like Vite to see if they're affected, too. There's a good chance they may be, too.
- What are the actual consequences of this issue?
Fast Refresh is broken on the pages where bson is imported. Fast Refresh is a huge boon to React devs, it keeps you in the flow and saves a lot of time from having to get the UI back to the state you're changing every time you tweak something. It's not a matter of just reloading extra modules. I know on my team at least this change has saved on the order of 30-60 minutes per day for the folks who were working on the complex interfaces in our application where Fast Refresh has been broken. This is a situation where it takes roughly 60 seconds to get the UI to a specific state, multiplied by 30-60 tweaks in that day.
- Is there truly no workaround here? @nbbeeken suggested perhaps targeting the CJS module and using a crypto polyfill might resolve the issue. Or something else?
I tried targeting the CJS module as I saw suggested in your other issues first, by overriding the resolve of the webpack config, but that didn't work, it broke the build for some reason. Our current workaround is to apply the changes from this PR as a patch to the bson package in our project.
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]?
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 vite-plugin-top-level-await plugin just because of bson. Then there are been issues like this with the plugin, completely breaking the build in some cases.
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.
Hey @lourd and @rkistner, apologies for the momentary radio silence, we are going to consider the change. We must look into additional testing to ensure this change is safe for all the platforms and products we support. We have current items in flight that need the team's attention but this is next up on our agenda, so stay tuned, thank you for your patience
Hi @lourd, thanks for the contribution! To accept these changes, however, we'd need you to add testing specifically for the behaviour you're adding here. As it is now, we could add a top-level await to the browser bundle and none of the existing tests would fail so it would be easy for us to accidentally reintroduce this behaviour in future.
Ok @W-A-James. I went ahead and pushed a commit removing the topLevelAwait config from the Webpack bundling test. If top level await was used once more I would expect that bundler test to now fail. But, to double check my assumption, I ran the bundling test on main without my changes, just removing that configuration, and the bundling worked without an issue.
Is there a different testing strategy I should use? One could go very blunt, writing a test that uses typescript or babel or something to create an AST of each of the files and throw an error if a top-level await statement exists, but that seems like overkill.