-
Notifications
You must be signed in to change notification settings - Fork 294
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
Add ESM support #186
Add ESM support #186
Conversation
Awesome, thanks! Yes, everything is built manually. Is there a way to test it? Manually, or in an automated way. |
@@ -1,3 +1,5 @@ | |||
var nacl = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This unfortunately pollutes global scope. Perhaps, I should move all this modularization stuff into a a build step, like I did here: https://github.com/dchest/fast-sha256-js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand what you did in fast-sha-256-js though. I thought exports in ESM were reserved for the top level as they are statically resolved, not dynamic. Would that still work as a direct import in a browser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, they are for CommonJS, but this can also be used when directly included using <script> tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant for ESM (edited my comment before I saw you reply), can ESM export
be dynamic or non-top-level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, no I don't think it can be dynamic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My hope is to be able to use this in a browser without any build step, so if that is possible while still keeping test and code duplication at an okay amount that'd be awesome.
If not I'll make it so some other way 🙂
Thanks for looking at and considering this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thanks! I think I'll make a special build step that will insert those things and generate .mjs files.
I've been testing it by manually modifying the tests mostly, I didn't want to be too invasive and changing the whole testsuite. For example this for a hash test:
I think the problem is how to keep CommonJS compatability while still running tests on ESM files, especially when tests themselves include certain dependencies. One solution would be to build separate versions and then run tests against those, but that sort of defeats the purpose of having native modules. Please let me know if you have an idea on how I can keep compatibility but still test ESM though, that'd be great :) |
Thanks, I'll figure something out for testing. |
Any progress? |
Closing since this is probably not getting merged and there are probably better (but also more invasive) ways to do it. |
Thank you very much for your changes - although they did not make it into the original code, they still allowed me to make tweet-nacl.js ESM compatible in my own fork within a few minutes! |
This is a pretty small change (looks large because it includes the generated files) that adds an export into ESM (also known as es6 modules).
I couldn't figure out where the minified build jobs are triggered, but the
build-esm
job should probably be added there too (or is it done manually when a release is done?).I think this meets the minimum to fix #170.
I didn't think this was large enough the warrant an AUTHORS.md inclusion, but let me know if that is needed/warranted.
It can probably be done a lot better in the future, but this was the least invasive way I could think of.