-
Notifications
You must be signed in to change notification settings - Fork 130
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
importing alea as a module #31
Comments
Is there any reason why using it as indicated in the readme wouldn't get the job done already:
? |
Yes, simplex-noise is getting the job done, in the sense that it has a great api, is performant, and produces good random number values. What I'm after additionally is not bundling duplicate alea implementations in my application. I have other random number related functions unrelated to simplex-noise that rely on https://www.npmjs.com/package/alea So ideally we could re-use the same alea module, as it means that much less duplicate code in my simulation. Thanks for a great module btw! |
Ah, I see. I guess the best solution to that would be to modernize the layout of the project a bit to support tree shaking. Might be as simple as setting "sideEffects": false and splitting up the code into separate files. Not a priority right now but could be a nice optimization. :) |
@jwagner I put together a proof of concept PR for this in #32 along with a number of other modernization changes. Would love to hear your thoughts, if this is something you think is worth pursuing. One thing to note: the PR is currently a "pure" es module, which means it only works in node 12.17 and up. It probably makes sense to produce a "dual" package, where we offer both an es module and a traditional commonjs (node) module for the foreseeable future. I'm happy to add this change, just didn't want to expend the effort yet since it's unclear if you like where this is going. Happy to add that if you think it's worthwhile. |
interesting and related: https://blog.sindresorhus.com/get-ready-for-esm-aa53530b3f77 That guy maintains a lot of npm modules and he's also advocating for going "pure es module" route. |
Hey Mike, Sorry for the late reply. A lot going on at work and more often than not too tired afterwards and I don't really like commenting without properly understanding the subject. Please don't take this as a lack of appreciation for your contribution. Interesting article by sindre. While I have a lot of understanding for his position I don't feel comfortable forcing users to move to ESM in order to import that package. I don't see the benefits (especially for a package as simple as this one) outweighing the disruption to users. It looks like the most sensible approach to keep compatibility is to keep the common js package mostly as it is (except for requiring alea) and including a simple esm wrapper that just reexports as described in this post by redfin. On a side note, that picture in that article by Sindre is giving me flashbacks. Either I'm having a dejavue or that picture was taken at web rebels 2012 in Oslo. |
All good! I know how you feel, I'm working a full time gig too. I'm fortunate in that they let me do some open source contribution during the day, otherwise there's no way I'd probably have time to do any. :)
Yeah, I agree with that too. For the time being, a dual package approach is safer and makes sense. When I say it's interesting, I mean from the perspective of how the landscape will probably see a dramatic shift this year, given how prolific that dude is as a module author. :) |
as of today, all supported versions of node.js natively handle esm modules. Might be worth re-considering now? |
It looks like build.sh is building a commonjs version for node; does it matter if node.js supports esm or not? |
I think we the new build setup esm support is not a major issue anymore since there is a fallback path so the ESM support is not a blocker anymore. So I think this could be worth a try now @mreinstein. Would also address #49. Just need to find a bit of time to give this a shot to see whether using the alea npm module would result in any compatibility or performance/size regressions. |
this PR should work ☝️ would appreciate some 👀 |
Closing this since alea has been removed. :) |
https://www.npmjs.com/package/alea is tiny and could probably be leveraged inside of
simplex-noise
.This would be valuable because other parts of my simulation rely on alea, and if simplex-noise is pulling this in as a dependency I'll have less duplicated code.
@jwagner would you be open to a PR for this? I'm happy to submit one.
The text was updated successfully, but these errors were encountered: