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(utils): fix warning thrown by Webpack #2843

Merged

Conversation

istateside
Copy link
Contributor

#2840 (comment)

This PR updates a line added in the recent 4.1.3 release. Webpack is throwing a warning when this library is included in a bundle, pointed at the given line.

I believe what's happening here is that Webpack sees the word "require", but doesn't understand that it's not being used to actually "require" something. Webpack doesn't know what to do with it, so it throws a warning.

The line is already wrapped in a try, so it should be fine to just drop the && require in the condition. If const nodeCrypto = require('crypto') fails, the error will get caught and we can continue on.

Closes issue: #2840

@istateside istateside requested a review from a team as a code owner March 16, 2021 20:41
@CLAassistant
Copy link

CLAassistant commented Mar 16, 2021

CLA assistant check
All committers have signed the CLA.

dylanb
dylanb previously approved these changes Mar 16, 2021
Fixes a warning thrown by Webpack for using "require" without importing
anything. Warning reads "require function is used in a way in which
dependencies cannot be statically extracted"

Closes issue dequelabs#2840
@istateside istateside force-pushed the fix-static-dependency-warning branch from d907879 to 8a68a82 Compare March 16, 2021 20:54
@istateside istateside changed the title Fix warning thrown by Webpack for using "require" without importing anything fix(utils): fix warning thrown by Webpack Mar 16, 2021
@@ -24,7 +24,7 @@ if (!_rng && _crypto && _crypto.getRandomValues) {
}

try {
if (!_rng && require) {
if (!_rng) {
Copy link
Member

Choose a reason for hiding this comment

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

This change worries me. How does this change effect other supported environments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before, if require wasn't defined, it would skip the conditional and continue on. Now, if require isn't defined, an error will be thrown by require('crypto') on the next line, which will be caught by the try/catch block. I don't think this changes the execution in a meaningful way

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation.

@dylanb
Copy link
Contributor

dylanb commented Mar 18, 2021

@WilcoFiers please merge if you are ok with this too

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. Yeah this makes sense.

@WilcoFiers WilcoFiers merged commit 0826177 into dequelabs:develop Mar 19, 2021
straker pushed a commit that referenced this pull request Apr 1, 2021
Fixes a warning thrown by Webpack for using "require" without importing
anything. Warning reads "require function is used in a way in which
dependencies cannot be statically extracted"

Closes issue #2840
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants