fix(NODE-5152): remove harmful/unecessary RequireRewriter rollup transformer

[minenwerfer]

Description

Current ESM packaging with rollup uses an unecessary plugin to wrap nodejsRandomBytes, a function that tries to require crypto and then fallback to another function, around an top-level async/await anonymous function. According to the documentation provided in the source:

"crypto" is the only dependency BSON needs. This presents a problem for creating a bundle of the BSON library in an es module format that can be used both on the browser and in Node.js. In Node.js when BSON is imported as an es module, there will be no global require function defined, making the code below fallback to the much less desireable math.random bytes. In order to make our es module bundle work as expected on Node.js we need to change this require() to a dynamic import, and the dynamic import must be top-level awaited since es modules are async. So we rely on a custom rollup plugin to seek out the following lines of code and replace require with await import and the IIFE line (nodejsRandomBytes = (() => { ... })()) with nodejsRandomBytes = await (async () => { ... })() when generating an es module bundle.

This is in fact untrue as as of March 2023 rollup is capable of handling this function as-it-is and the source transformation actually produces unexpected build-time and runtime behaviors.

What is changing?

Rollup transformation to wrap src/utils/node_byte_utils.ts:nodejsRandomBytes around an async/await function is being deleted.

Is there new documentation needed for these changes?

No.

What is the motivation for this change?

This entire transformation is useless as browser and node modules are capable of handling the quoted function without it seamlessly. I'm using bson package in both environments (in an webapp bundled with rollup and in the node backend). Before the change I was unable to bundle my webapp for production without throwing on build (worked around this by converting the package to cjs first within rollup config) and then it threw on runtime also (browser interpreter, Chromium by the way, threw because the code generated by rollup put the await in an invalid context). After the change both my applications (packed with ESM) are running as expected, no exceptions thrown and no need to convert to CJS.

I can create a vite reproduction repo if needed, just let me know.

[nbbeeken]

Hi @ringeringeraja thank you for your submission, I've created a related JIRA ticket here: https://jira.mongodb.org/browse/NODE-5152

[minenwerfer]

Tests are passing by the way.

[nbbeeken]

Hi @ringeringeraja, the proposed change leaves a require call present in the .mjs bundle, since require does not exist when using ES Modules in Node.js it will throw and fallback to use Math.random which is not acceptable for most users who need cryptographic random number generation.

This is in fact untrue as as of March 2023 rollup is capable of handling this function

Can you point me in the direction of what this is referring to, March 2023 is last month so is there a recent rollup feature that relates to the translation of this syntax? The version of rollup this package currently uses is from 4 months ago (3.7.1)

[minenwerfer]

Hi! I'm horribly sorry, it was all my misunderstanding. The changes I made solved my problem in the browser, but the function did indeed fallback to Math.random in Node as I verified later (that's what the comment says).

So here's how I solved: I created an alias in Vite to forcefully resolve 'bson' to the .cjs bundle, in fact there was no relevant tree
shaking being made anyway, so it worked fine.

I'm closing this PR now, thanks!

[nbbeeken]

Glad it worked out! That does sound like the right approach to me, especially if combined with require.resolve('bson') to make the path resolution robust cross platforms.

For the sake of any others that may come across this issue, do you mind sharing the alias settings? Is this: https://vitejs.dev/config/shared-options.html#resolve-alias the correct documentation?

[minenwerfer]

Here's my working config:

import { defineConfig } from 'vite'

export default defineConfig({
  resolve: {
    alias: {
      'bson': require.resolve('bson')
    }
  },
  optimizeDeps: {
    include: [
      'bson'
    ]
  },
})

Then call it like:

NODE_PATH=../../node_modules npx vite build

[minenwerfer]

Hi @ringeringeraja, the proposed change leaves a require call present in the .mjs bundle, since require does not exist when using ES Modules in Node.js it will throw and fallback to use Math.random which is not acceptable for most users who need cryptographic random number generation.

This is in fact untrue as as of March 2023 rollup is capable of handling this function

Can you point me in the direction of what this is referring to, March 2023 is last month so is there a recent rollup feature that relates to the translation of this syntax? The version of rollup this package currently uses is from 4 months ago (3.7.1)

@nbbeeken don't you think a new conditional export could be used to get around this issue without resorting to the asynchronous dynamic import? In bson.node.mjs the crypto package could be imported statically, maybe with another rollup transformation.

{
  "exports": {
    "node": {
      "import": "./bson.node.mjs",
      "require": "./bson.cjs"
    },
    ...
  }
}

Reference: https://nodejs.org/api/packages.html#nested-conditions

[nbbeeken]

It can get around it yes just as the bundler config settings can. I think shipping an additional copy of the library (currently at 4 copies) is not an ideal approach to solve this issue when each bundler we've encountered having a problem with this has a setting to fix it. I can't say for sure but I would assume the setting needed will eventually become a default as this is part of the language and may only become more commonplace.

Similar proposal: #669 (review)