fix(NODE-6730): allow webpack bundling #67

durran · 2025-02-06T17:10:04Z

Description

Updates require to work with Webpack.

What is changing?

Removes the double try/catch that attempts to require the debug native version, which would never be used in the published module.
Adds testing around bundling the module with Webpack.

> $ npm run build                                                                                      ⬡ 20.17.0 [±NODE-6730 ●]

> [email protected] build
> webpack --mode=production --node-env=production

asset 59a87aace1fec32b28c5abf2b6511033.node 5.68 MiB [compared for emit] (auxiliary name: main)
asset main.js 2.23 KiB [emitted] [minimized] (name: main)
runtime modules 150 bytes 2 modules
cacheable modules 4.25 KiB
  modules by path ./node_modules/mongodb-client-encryption/lib/*.js 3.94 KiB
    ./node_modules/mongodb-client-encryption/lib/index.js 1.28 KiB [built] [code generated]
    ./node_modules/mongodb-client-encryption/lib/crypto_callbacks.js 2.65 KiB [built] [code generated]
  ./src/index.ts 128 bytes [built] [code generated]
  ./node_modules/mongodb-client-encryption/build/Release/mongocrypt.node 197 bytes [built] [code generated]
external "crypto" 42 bytes [built] [code generated]
external "path" 42 bytes [optional] [built] [code generated]

WARNING in ./node_modules/mongodb-client-encryption/lib/index.js 12:19-60
Module not found: Error: Can't resolve '../build/Debug/mongocrypt.node' in '/Users/modetojoy/work/mongodb-js/mongodb-client-encryption/test/bundling/webpack/node_modules/mongodb-client-encryption/lib'
 @ ./src/index.ts 1:0-55 3:16-26

1 warning has detailed information that is not shown.
Use 'stats.errorDetails: true' resp. '--stats-error-details' to show it.

webpack 5.97.1 compiled with 1 warning in 871 ms

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Package is now compatible with Webpack

In order to use this module when bundling with Webpack, please include in your webpack.config.js:

  target: 'node',
  module: {
    rules: [
      {
        test: /\.node$/i,
        loader: 'node-loader'
      }
    ]
  }

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

durran · 2025-02-06T22:55:50Z

test/bundling/webpack/webpack.config.js

+    path: path.resolve(__dirname, 'dist')
+  },
+  experiments: { topLevelAwait: true },
+  target: 'node',


This line in combination with lines 20 & 21 are they key to get the bundle to work.

durran · 2025-02-07T00:25:42Z

src/index.ts

@@ -5,7 +5,8 @@ function load() {
  try {
    return require('../build/Release/mongocrypt.node');
  } catch {
-    return require('../build/Debug/mongocrypt.node');
+    // eslint-disable-next-line no-console
+    console.log('Could not load the native module mongocrypt.node');


The Debug native binding is never published by us, so no reason to attempt to include it in the published package and generate a warning from Webpack.

Suggested change

console.log('Could not load the native module mongocrypt.node');

try {

return require('../build/Debug/mongocrypt.node');

} catch (error) {

throw error;

}

It is kinda nice to have the code set up to pull in the debug build when dev-ing locally. So all we need here is a try {} around the require

You may see "Module not found: Error: Can't resolve" but it is printed as a "WARNING" and does not stop the build and that's the intended behavior

I had a conversation with Bailey about this and we thought it was more developer friendly to not have a warning even in the event of a valid bundle. I'm on that side that the bundle should be warning-free, since the debug aspect is only for ourselves and would never be published.

Also just to clarify with the double try/catch, Webpack will still issue a warning about the Debug module not being available, though it will not fail. I think it's a better developer experience that if they bundle the module correctly, no warning is issued.

The warning is desirable IMO because each warning alerts you to what your bundle is missing, you can squash them in your webpack config if you don't want to see them and it creates an intentional "paper trail" of the resolutions you don't care about.

We may not ship Debug but it can still be built and imported if Release is removed.

If we really feel strongly about not having Debug that's fine but we shouldn't introduce a console.log on import when this is missing, we can just throw inside this catch. The module is going to throw anyway when mc is accessed during extends mc.MongoCrypt

RFC @addaleax, our resident webpack user 😅

Yeah, definitely feeling strongly that:

We should support the debug-mode addon, running npm install --debug --build-from-source ... isn't that hard and it's usually what a (sufficiently experienced) user will do to start diagnosing issues with native addons

require()ing a native addon package should throw if the addon itself is unavailable – that's just the general expectation for how addon packages behave

A warning at runtime printed via console.error() is much more disruptive than a warning in webpack. Most webpack configs will result in a bunch of warnings being printed, that's unfortunately more or less a fact of life

I've updated to throw now if both modules are missing.

durran · 2025-02-07T00:26:58Z

test/unit/bindings.test.ts

@@ -1,5 +1,5 @@
 import { expect } from 'chai';


I moved the root tests to test/unit to avoid all the root test/ eslint craziness.

src/index.ts

Co-authored-by: Neal Beeken <[email protected]>

nbbeeken · 2025-02-07T17:26:30Z

Despite eslint's warning, it is not a useless catch 😅 given all that it is solving for here

durran added 5 commits February 6, 2025 23:44

fix(NODE-6730): allow webpack bundling

1fe6088

chore: add git ignore

c16a085

chore: update readme

e77cc7f

test: move to unit dir

86da582

fix: eslint

6a6773e

durran force-pushed the NODE-6730 branch from 8046684 to 6a6773e Compare February 6, 2025 22:44

durran commented Feb 6, 2025

View reviewed changes

durran added 2 commits February 7, 2025 00:22

chore: add github action for webpack

7799f4f

chore: rename workflow

896b569

durran marked this pull request as ready for review February 7, 2025 00:14

durran requested a review from a team as a code owner February 7, 2025 00:14

durran commented Feb 7, 2025

View reviewed changes

fix: throw if no debug

3d173cc

baileympearson self-assigned this Feb 7, 2025

baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Feb 7, 2025

nbbeeken requested changes Feb 7, 2025

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

nbbeeken assigned nbbeeken and unassigned baileympearson Feb 7, 2025

Update src/index.ts

d7e9f74

Co-authored-by: Neal Beeken <[email protected]>

addaleax approved these changes Feb 7, 2025

View reviewed changes

chore: document catch and rethrow

f52f714

nbbeeken approved these changes Feb 7, 2025

View reviewed changes

nbbeeken merged commit 3787edd into main Feb 7, 2025
30 of 31 checks passed

nbbeeken deleted the NODE-6730 branch February 7, 2025 17:40

github-actions bot mentioned this pull request Feb 6, 2025

chore(main): release 6.2.0 #61

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(NODE-6730): allow webpack bundling #67

fix(NODE-6730): allow webpack bundling #67

durran commented Feb 6, 2025 •

edited

Loading

durran Feb 6, 2025

durran Feb 7, 2025

nbbeeken Feb 7, 2025

durran Feb 7, 2025

durran Feb 7, 2025

nbbeeken Feb 7, 2025

nbbeeken Feb 7, 2025

addaleax Feb 7, 2025 •

edited

Loading

durran Feb 7, 2025

durran Feb 7, 2025

nbbeeken commented Feb 7, 2025

fix(NODE-6730): allow webpack bundling #67

fix(NODE-6730): allow webpack bundling #67

Conversation

durran commented Feb 6, 2025 • edited Loading

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Package is now compatible with Webpack

Double check the following

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbbeeken commented Feb 7, 2025

durran commented Feb 6, 2025 •

edited

Loading

addaleax Feb 7, 2025 •

edited

Loading