-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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-4425): webpack optional import of FLE issue #3324
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a test for fle to optional_require.test.js
?
From slack, decided to follow up with this: https://jira.mongodb.org/browse/NODE-4442 |
src/utils.ts
Outdated
|
||
// NOTE(NODE-4254): This is to get around the circular dependency between | ||
// mongodb-client-encryption and the driver in the test scenarios. | ||
if ( | ||
typeof process.env.MONGODB_CLIENT_ENCRYPTION_OVERRIDE === 'string' && | ||
process.env.MONGODB_CLIENT_ENCRYPTION_OVERRIDE.length > 0 | ||
) { | ||
mongodbClientEncryption = require(process.env.MONGODB_CLIENT_ENCRYPTION_OVERRIDE); | ||
try { | ||
// NOTE(NODE-3199): Ensure you always wrap an optional require in the try block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As someone who read this comment and didn't realize that the require must literally be inside the try catch, can we update the comment to be more descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to address lint failures
Description
What is changing?
Move FLE require back into a try catch block.
What is the motivation for this change?
Webpack needs optional requires to be inside of a try catch.
Double check the following
npm run check:lint
script<type>(NODE-xxxx)<!>: <description>