-
Notifications
You must be signed in to change notification settings - Fork 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
Error with webpack 5 #68
Comments
Would you like to submit a pull request for this @dmytro-podolianskyi-ew ? |
@dmytro-podolianskyi-ew you might want to use https://www.npmjs.com/package/generate-password-browser |
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. |
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? |
@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? |
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. |
@davidrenne do you want to submit a pull request to switch to the randombytes package? Happy to approve it. |
when using webpack 5, I made it by using 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 |
I mean, that's exactly what I did for the generate-password-browser fork 😄 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. |
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 |
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
},
The text was updated successfully, but these errors were encountered: