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

importing alea as a module #31

Closed
mreinstein opened this issue Dec 6, 2020 · 12 comments
Closed

importing alea as a module #31

mreinstein opened this issue Dec 6, 2020 · 12 comments

Comments

@mreinstein
Copy link

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
Copy link
Owner

jwagner commented Dec 31, 2020

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
Copy link
Author

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
Copy link
Owner

jwagner commented Dec 31, 2020

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
Copy link
Author

@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
Copy link
Author

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
Copy link
Owner

jwagner commented Jan 15, 2021

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
Copy link
Author

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
Copy link
Author

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

@mreinstein mreinstein reopened this May 16, 2022
@redblobgames
Copy link

redblobgames commented May 17, 2022

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

@jwagner
Copy link
Owner

jwagner commented May 17, 2022

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
Copy link
Author

this PR should work ☝️ would appreciate some 👀

@jwagner
Copy link
Owner

jwagner commented Jul 23, 2022

Closing this since alea has been removed. :)

@jwagner jwagner closed this as completed Jul 23, 2022
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

No branches or pull requests

3 participants