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

Replace seed-random by seedrandom #1955

Merged
merged 1 commit into from
Sep 6, 2020

Conversation

poppinlp
Copy link
Contributor

@poppinlp poppinlp commented Sep 1, 2020

For issue #1949 , replace the seed-random by seedrandom.

  • I have no idea about which is better of the fast PRNG algorithms supported by seedrandom, so I choose the default.
  • The bundle size increases a little, so I change the maxSize up.

I've run the build-and-test locally. Looks no error.

@poppinlp poppinlp force-pushed the replace-seed-random branch from 9ed64b7 to f254da1 Compare September 2, 2020 06:53
@josdejong
Copy link
Owner

Thanks Poppin! That looks like a very easy drop in replacement.

Do you know how much bigger the new seedrandom library is compared to the old one?

I'll do some testing with your PR hopefully coming weekend.

@poppinlp
Copy link
Contributor Author

poppinlp commented Sep 3, 2020

Compare seedrandom deliver js with seed-random index.js, it's 3K bigger around.
So, I guess, it's the reason that the bundle size increase from 98494 to >10000 which will break the current limit condition. T_T

@josdejong josdejong mentioned this pull request Sep 6, 2020
4 tasks
@josdejong
Copy link
Owner

Thanks for checking, +3K should be fine I think (and when minified/compressed it's much smaller).

I'll merge your PR now in the v8 branch I just made. It may be that people rely on specific behavior or methods of the old library, so I think we should handle it as a breaking change.

@josdejong
Copy link
Owner

This fix is published now in v8.0.0, thanks again @poppinlp

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.

2 participants