jwagner / simplex-noise.js

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

use pure es module, use alea from npm, modernize module. fixes #31 #32

Closed mreinstein closed 3 years ago

mreinstein commented 3 years ago

a number of changes:

jwagner commented 3 years ago

Hey Mike,

Thanks a lot for the PR. As mentioned before I really haven't looked into ES modules in detail yet but from what I can tell this change would break Node < 13.2, about 7% of browsers and require something along the lines of NPM and webpack. As is I don't think treeshaking would work (as I don't think it works on a per method level) so the savings would just be the ~500 bytes for the alea implementation.

How do you see the trade off of moving to es modules?

Cheers, Jonas

mreinstein commented 3 years ago

Hi Jonas!

this change would break Node < 13.2

It will work on node v12.17 and up. It's marked as experimental but the node team has removed the need for the command line flag and removed the experimental warning too. So the only maintained version of node right now that lacks support is 10.x, and that will be unmaintained starting in April.

But yes, even though the support is fairly good, it would still be a breaking change. A simple solution for this is to release what is called a "dual package", where npm has both an es module and a commonjs (node) module for a while.

I'm happy to add the dual package support into the PR, I just didn't want to bother with that until you had a chance to give some initial feedback. Shall I add that in now?

so the savings would just be the ~500 bytes for the alea implementation.

It's true that we're not saving a huge number of bytes, but also keep in mind that your concisely written simplex-noise module is not more than a few kilobytes. So 500 bytes is actually a pretty good savings (I think something around 10% smaller package.) Every little bit counts, especially since projects that use npm tend to accumulate a lot of modules. :)

redblobgames commented 2 years ago

(Yes I realize this is an old PR)

mreinstein: I think if we want to save more bytes (and I occasionally do), the biggest savings would be to split noise2D, noise3D, noise4D out into separate tree-shakable functions or classes, and then also having alea passed in as an argument, not something SimplexNoise imports on its own. That way if I'm using something other than alea, I don't need to include the alea and hash functions in the bundle. And if I'm not using noise4D I don't have to import that, and the bundler wouldn't include it.

Quick test—

*for projects that aren't using these functions

On the other hand it sure is convenient that the module includes all of this in one nice interface. So maybe there's some way to structure it so that the main module imports alea, permutations, noise2d, noise3d, noise4d from submodules and puts them together into the SimplexNoise class (both for convenience and for node.js), and then someone who is byte sensitive can import the wanted esm submodules directly.

mreinstein commented 2 years ago

hey Amit, great to see you again! Reminds me to go check your blog for new articles, it's been a minute :D

jwagner commented 2 years ago

@redblobgames I played with that idea as well when I moved to project over to esm and added typescript support. My main reason for not doing it yet is that it would make explaining the interface a bit more complicated. The docs would need to say something along the lines of 'there is a single interface based approach over here that you can use, and then there is a more modular approach over there that you might consider using if only use a subset of the library and care about your bundle sizes a lot. Not a very strong argument though so it's definitely worth considering adding a tree-shakeable interface to the next version.