jwagner / simplex-noise.js

A fast simplex noise implementation in Javascript / Typescript.
MIT License
1.61k stars 130 forks source link

importing alea as a module #31

Closed mreinstein closed 2 years ago

mreinstein commented 3 years ago

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.

jwagner commented 3 years ago

Is there any reason why using it as indicated in the readme wouldn't get the job done already:

var random = new Alea(seed),
    simplex = new SimplexNoise(random),
    value2d = simplex.noise2D(x, y);

?

mreinstein commented 3 years ago

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. simplex-noise is "baking in" an alea implementation here: https://github.com/jwagner/simplex-noise.js/blob/master/simplex-noise.js#L409-L463

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!

jwagner commented 3 years ago

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. :)

mreinstein commented 3 years ago

@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.

mreinstein commented 3 years ago

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.

jwagner commented 3 years ago

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.

mreinstein commented 3 years ago

Hey Mike, Sorry for the late reply.

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. :)

I don't feel comfortable forcing users to move to ESM in order to import that package

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. :)

mreinstein commented 2 years ago

as of today, all supported versions of node.js natively handle esm modules. Might be worth re-considering now?

redblobgames commented 2 years ago

It looks like build.sh is building a commonjs version for node; does it matter if node.js supports esm or not?

jwagner commented 2 years ago

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.

mreinstein commented 2 years ago

this PR should work ☝️ would appreciate some 👀

jwagner commented 2 years ago

Closing this since alea has been removed. :)