Skip to content

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

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

Merged
merged 10 commits into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"clang-format": "clang-format --style=file:.clang-format --Werror -i addon/*",
"check:eslint": "ESLINT_USE_FLAT_CONFIG=false eslint src test",
"check:clang-format": "clang-format --style=file:.clang-format --dry-run --Werror addon/*",
"test": "mocha --v8-expose-gc test",
"test": "mocha --v8-expose-gc test/unit",
"prepare": "tsc",
"rebuild": "node-gyp rebuild",
"prebuild": "prebuild --runtime napi --strip --verbose --all"
Expand Down
3 changes: 2 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC @addaleax, our resident webpack user 😅

Copy link
Collaborator

@addaleax addaleax Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, definitely feeling strongly that:

  1. 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
  2. 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
  3. 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

Expand Down
1 change: 1 addition & 0 deletions test/bundling/webpack/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
dist/*
24 changes: 24 additions & 0 deletions test/bundling/webpack/install_ce.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
'use strict';

const { execSync } = require('node:child_process');
const { readFileSync } = require('node:fs');
const { resolve } = require('node:path');

const xtrace = (...args) => {
console.log(`running: ${args[0]}`);
return execSync(...args);
};

const ceRoot = resolve(__dirname, '../../..');
console.log(`mongodb-client-encryption package root: ${ceRoot}`);

const ceVersion = JSON.parse(
readFileSync(resolve(ceRoot, 'package.json'), { encoding: 'utf8' })
).version;
console.log(`mongodb-client-encryption Version: ${ceVersion}`);

xtrace('npm pack --pack-destination test/bundling/webpack', { cwd: ceRoot });

xtrace(`npm install --no-save mongodb-client-encryption-${ceVersion}.tgz`);

console.log('mongodb-client-encryption installed!');
Loading
Loading