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

Add ESM support #186

Closed
wants to merge 1 commit into from
Closed

Add ESM support #186

wants to merge 1 commit into from

Conversation

SvanteRichter
Copy link

@SvanteRichter SvanteRichter commented Jan 27, 2020

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.

@dchest
Copy link
Owner

dchest commented Jan 27, 2020

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 = {};
Copy link
Owner

@dchest dchest Jan 27, 2020

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

Copy link
Author

@SvanteRichter SvanteRichter Jan 27, 2020

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?

Copy link
Owner

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.

Copy link
Author

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?

Copy link
Owner

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.

Copy link
Author

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!

Copy link
Owner

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.

@SvanteRichter
Copy link
Author

Is there a way to test it? Manually, or in an automated way.

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:

import nacl from './nacl-fast.mjs';
import util from 'tweetnacl-util';
import test from 'tape';
nacl.util = util;

var enc = nacl.util.encodeBase64,
    dec = nacl.util.decodeBase64;

let randomVectors = /* insert/import ./data/hash.random.js here */

test('nacl.hash random test vectors', function(t) {
  randomVectors.forEach(function(vec) {
    var msg = dec(vec[0]);
    var goodHash = dec(vec[1]);
    var hash = nacl.hash(msg);
    t.equal(enc(hash), enc(goodHash));
  });
  t.end();
});

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

@dchest
Copy link
Owner

dchest commented Jan 27, 2020

Thanks, I'll figure something out for testing.

@Saiv46
Copy link

Saiv46 commented Jun 19, 2020

Any progress?

@SvanteRichter
Copy link
Author

Closing since this is probably not getting merged and there are probably better (but also more invasive) ways to do it.

@rozek
Copy link

rozek commented Jan 4, 2024

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!

@SvanteRichter
Copy link
Author

@rozek No problem, nice to see somebody using it! Seems like there are a couple of different forks that have done this now, maybe list yours also on #170.

Repository owner deleted a comment Mar 10, 2024
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

Successfully merging this pull request may close these issues.

ES6 modules
4 participants