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): Removes top-level await in bson with separate node and browser ESM bundles #749

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lourd
Copy link

@lourd lourd commented Feb 13, 2025

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 developing

This 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

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

@lourd lourd requested a review from a team as a code owner February 13, 2025 06:47
@lourd
Copy link
Author

lourd commented Feb 13, 2025

Pinging @nbbeeken for review 🙏

@dariakp dariakp added tracked-in-jira There is a ticket in Mongo's Jira instance tracking this issue/PR External Submission labels Feb 13, 2025
@dariakp
Copy link
Contributor

dariakp commented Feb 13, 2025

@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.

@baileympearson
Copy link
Contributor

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]?

@lourd
Copy link
Author

lourd commented Feb 21, 2025

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.

@rkistner
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Submission tracked-in-jira There is a ticket in Mongo's Jira instance tracking this issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants