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

Error with webpack 5 #68

Open
dmytro-podolianskyi-ew opened this issue Nov 23, 2021 · 10 comments
Open

Error with webpack 5 #68

dmytro-podolianskyi-ew opened this issue Nov 23, 2021 · 10 comments

Comments

@dmytro-podolianskyi-ew
Copy link

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it

If you want to include a polyfill, you need to:
- add a fallback 'resolve.fallback: { "crypto": require.resolve("crypto-browserify") }'
- install 'crypto-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
resolve.fallback: { "crypto": false }

Please, add to package.json this option

"browser": {
"crypto": false
},

@brendanashworth
Copy link
Owner

Would you like to submit a pull request for this @dmytro-podolianskyi-ew ?

@beardsleym
Copy link

@dmytro-podolianskyi-ew you might want to use https://www.npmjs.com/package/generate-password-browser

@davidrenne
Copy link

davidrenne commented Sep 1, 2022

Yes I think I encountered this same issue as we are on webpack 4 in one project I work on. I was lazy loading several components which also was including this and I got a huge package of browserify and browserify-crypto and browserify-sign and browserify-rsa (asn1.js also I think was a dependency to load this require) which bloated my bundle dramatically.

I found this library to be the culprit so just ripped it out and wrote a backend handler for generating the password with go.

Was it because it uses var crypto = require('crypto');? where Webpack just doesnt know how to handle it and needs all these browserify helpers.

Is this just a node.js only library where we accidentally used it on web?

At least I stopped the bloat in its tracks and removed this library from our pipeline.

@davidrenne
Copy link

davidrenne commented Sep 1, 2022

This package gets downloaded a lot (Im sure by node.js backend developers) and I know the original author of node.js I thought said he doesnt like npm because it allows importing of node modules for things on the web that werent intended for the web. Or maybe it was something about create-react-app allowing import of node.js modules for a react application on the client side.

This guy I think found the same issue with this library being used on the web accidentally:

https://stackoverflow.com/a/69105180/2040020

Not sure if @brendanashworth can do much about people using it on the web and then browserify and all it's dependencies being bundled accidentally bloating bundles because of this library is used only for backend? I guess the crypto package is like the fs package, backend only intended usage, right?

I suppose a warning on install? "This package should only be used in node.js backend projects otherwise browserify might include many dependencies for crypto and increase your front-end bundle size and vendors", or perhaps something in the readme about being a backend node.js only library.

I am not a node.js expert, mostly front-end/react/webpack and go, but arent you just trying to generate a randomy bytestring of 256 bytes.

https://github.com/nodejs/node/blob/main/lib/internal/crypto/random.js#L99

I realize this is using fastBuffer, but cant you just write your own byte randomizer and reduce this dependency so people on the web can use this library without all the browserify stuff loading because of this crypto require?

@brendanashworth
Copy link
Owner

@davidrenne yikes interesting link to that stackoverflow... am not sure how to reduce the bundled size for people who use crypto-browserify. Is there a way to signal that we only use randomBytes?

I do not want to fall back on Math.random because that is not cryptographically secure and developers should expect this library to be safe to use out of the box.

Writing our own random number generator would probably be overkill for a library like this. Is there any way to signal to browserify that we only use one function of crypto?

@davidrenne
Copy link

davidrenne commented Sep 4, 2022

From my understanding crypto is a part of node.js and not a separate package. Typically with a package with separate files you can import certain files which reduces the bundle size but I think you cant because you need crypto to do what you do in your package.

I found this:

https://www.npmjs.com/package/randombytes

It seems they use crypto.randomBytes too but are importing possibly just crypto.randomBytes which possibly wouldnt be including all of crypto like your package does:

https://github.com/crypto-browserify/randombytes/blob/master/index.js#L1

And then in their browser.js they are using msCrypto or crypto, but in my browser chrome latest I have window.crypto.getRandomValues as a function and not msCrypto, so I think my case would use global.crypto coming from window when running in the browser.

Where your package imports all of crypto, which makes sense because I saw a bunch of aes and other cryptographic function names in my bundle all over the place because of how my application was chunked and how this generate-password was being used, webpack started importing browserify and all its dependencies to run the full crypto package in the browser.

It was just frustrating because yarn list showed these as dependencies of webpack, but not any any library because your package.json isnt really using a package on npm, but a core library bundled in node. I wish yarn and npm did a better job of showing node.js requires outside of each package.json dependencies.

Sorry I am in a hurry and my battery is about to die. Sorry if this doesnt make sense 100%.

But overall it will be difficult for you to see this, unless you had a web app chunked and importing generate-password where you'd see what I saw. My app is a private repo, otherwise I would show you some of the analyzer stuff. Maybe I can do that if you are interested in seeing how our app loaded all this stuff due to this package I can email you the HTML output of. webpack analyzer if you are interested in trying to maintain your code, yet working better for people using it in the browser with webpack.

@brendanashworth
Copy link
Owner

@davidrenne do you want to submit a pull request to switch to the randombytes package? Happy to approve it.

@shawnzhu
Copy link

shawnzhu commented Aug 4, 2023

when using webpack 5, I made it by using node-polyfill-webpack-plugin) in webpack.config.js w/o any resolve.fallback:

const NodePolyfillPlugin = require('node-polyfill-webpack-plugin');

module.exports = {
	// Other rules...
	plugins: [
		new NodePolyfillPlugin()
	]
};

This is handy b/c it doesn't need to change existing code/runtime-dependency

@oliver-la
Copy link

@brendanashworth

do you want to submit a pull request to switch to the randombytes package? Happy to approve it.

I mean, that's exactly what I did for the generate-password-browser fork 😄
In case you changed your mind, I'm happy to submit a PR.

As a sidenote, I just noticed the "Using this with a browser" section in your README, which is a bit misleading. My fork (as of now) replaces the crypto module with randombytes only. The "require" method is still there and while browsers don't understand that method, bundlers (e.g. webpack) will take care of using something more appropriate. But it's not the reason the fork exists.

Sorry for digging up this over 3 year old issue, just noticed it.

@brendanashworth
Copy link
Owner

I mean honestly if the world's web bloat gets smaller if I add browser support maybe we should just do it? and release a minor bump and give browsers a smaller payload

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

No branches or pull requests

6 participants