search

dchest / tweetnacl-js

Port of TweetNaCl cryptographic library to JavaScript
https://tweetnacl.js.org
The Unlicense
1.75k stars 292 forks source link

Convert to ES modules #199

Closed jessetane closed 3 years ago

jessetane commented 3 years ago

This PR is a somewhat major reworking of how the package is organized and tested. It's pretty opinionated so my expectation is not necessarily to have it merged, so much as to discuss the general approach and potential problems. Below is a list of some of the most significant issues I came across and changes I made while getting this working.

  1. To keep complexity manageable, I propose eliminating all cjs and bumping a major version. Cjs folks can use the previous version, and important security fixes can be backported. Are there any good reasons to go through the trouble of trying to support cjs and mjs simultaneously?

  2. Rename nacl-fast.js -> index.js and nacl.js -> original.js. I find nacl-fast.js being the main file confusing... maybe that's just me, but having the main file be called index.js seems more standard?

  3. Move stuff that's not part of the test suite out to top-level folders (bench and timing).

  4. Remove some stuff:

    • lockfiles (my understanding is that these aren't as useful in a library context as they are in an end application).
    • any files for distribution, anything being minified (the main file(s) can be used as-is now, and minification of esm is maybe not quite ready for prime time).

  5. Use dynamic import for dependency injection. There are two main places this is happening:

    • The PRNG setup that uses crypto.randomBytes in node, and window.crypto.getRandomValues in the browser. This has the potential caveat (only when used in node) that a consumer could call a dependent method before the dynamic import completes - is there a better way?
    • Selecting which variant of the library to use in the tests (and benchmarks). An environment variable is still used as the selection mechanism in node, however dedicated entry point(s) have to be used now rather than the globs that were previously passed to the tape cli.

  6. Eslint doesn't support dynamic imports yet and this doesn't appear to be something that can be easily worked around - I pulled in the babel-eslint parser which seems to do the trick for now.

  7. Tape (the library used for testing) is not itself an es module and so it can't be used in the browser without a build step. To work around this, tap-esm has been used instead.

  8. Browser tests no longer require a build step, but es modules do require an http server (I pulled in ecstatic for this purpose). Query params can be used to control which tests are run, and which variant of the library to use.

  9. Use import maps in the browser to handle bare import specifiers. This feature has not been implemented by browsers yet, but there is a spec and a shim, which has been used here for the time being.

  10. Use package.json exports field to indicate the main file and enable self-referencing import statements. E.g. import nacl from 'tweetnacl' from within the package itself.

jessetane commented 3 years ago

Found out about top-level-await and managed to get this a little farther along. It's a lot less opinionated now, although it still does not attempt to support both cjs and mjs.