davidbau / seedrandom

seeded random number generator for Javascript
2.04k stars 160 forks source link

seedrandom is creating massive bundles when used with browserify #40

Closed mreinstein closed 7 years ago

mreinstein commented 7 years ago

I'm trying to seedrandom in browserify. The result works but creates a gigantic amount of javascript that gets put into the bundle.

My bundle size before seedrandom: 17975 bytes. after: 618002 bytes.

I this is happening because of checks like this: https://github.com/davidbau/seedrandom/blob/released/seedrandom.js#L232

browserify is technically runnning in a node environment so it includes this module, but then it ends up shimming the node crypto module for use in the browser, which isn't needed at all.

would love to figure out how we could solve this, as it seems like a high quality rng module but I can't inflate my front end javascript from 18k -> 618k.

thanks!

jozan commented 7 years ago

I fixed this on my fork.

https://github.com/jozan/seedrandom/releases/tag/v2.5.0

mreinstein commented 7 years ago

@jozan are you planning to send a PR back here?

davidbau commented 7 years ago

The interaction with browserify is unfortunate; but jozan's version removes the require('crypto') even when running in node.js.

Are there any idioms we can use to convince browserify to not bundle a dependency which still resolves under node? For example, could we say "var c = 'crypto'; require(c);" to disable browserify bundling in this case?

I'd love help with a PR that addresses the issue in some way like this.

mreinstein commented 7 years ago

jozan's version removes the require('crypto') even when running in node.js.

Ah, I see what you mean. :(

Are there any idioms we can use to convince browserify to not bundle a dependency which still resolves under node?

this might work: https://github.com/substack/browserify-handbook#ignoring-and-excluding

ideally this ignore/exclude doesn't need to happen in the module that includes seedrandom. It would be much better if we could put some directive in seedrandom's package.json to inform browserify to to not include crypto when bundling.

mreinstein commented 7 years ago

43 is my PR which attempts to solve this. My browser builds are significantly smaller now, and seedrandom seems to still work! It's not passing tests though. @davidbau would be open to feedback, happy to make more changes to get this merge-able.

mreinstein commented 7 years ago

@davidbau clean PR ^

davidbau commented 7 years ago

I merged #44 after modifiying it to ignore crypto in package.json 'browser' rather than stubbing it. Also modified the browerify test so that we can see that browserified.js is small.

Can you verify that 'master' as I've modified it works for you, before we close out this issue?

mreinstein commented 7 years ago

Can you verify that 'master' as I've modified it works for you, before we close out this issue?

@davidbau works great! I think the only issue is that lib/crypto.js is now dead code and not used anywhere. Probably should be removed.

Also, need a new build and npm publish.

davidbau commented 7 years ago

Great. I removed lib/crypto.js from master.

Will wait about a week to see if any other issues come up, and then will release a new version.

jozan commented 7 years ago

Thanks for fixing this! Cheers! 👍

mreinstein commented 7 years ago

@davidbau are you feeling like an npm release is ok to proceed with at this point?

ldd commented 7 years ago

I'm just here for people that might google the problem for webpack: adding

    node: {
        crypto: 'empty'
    },

solved the issue

mreinstein commented 7 years ago

@davidbau I hate to be a pest but any chance of a new npm version soon?

mreinstein commented 7 years ago

@davidbau can you please publish a new version of seedrandom to npm? I really prefer to point at modules via npm rather than github because they are versioned and archived and more generally available. pointing production projects at a specific github repo commit is not ideal.

davidbau commented 7 years ago

I think i published 2.4.3 to npm a couple weeks ago. Can you verify that it's OK? If something is wrong with 2.4.3, let's debug and I can release a 2.4.4.

mreinstein commented 7 years ago

I think i published 2.4.3 to npm a couple weeks ago.

argh! You're right. I guess I never noticed that a new version was available on npm. 2.4.3 works. Thanks, and sorry for the noise!