Skip to content
This repository has been archived by the owner on Feb 16, 2022. It is now read-only.

Use external noble/ed25519, fix coverage, remove config #132

Merged
merged 2 commits into from
Jan 10, 2022

Conversation

sgwilym
Copy link
Contributor

@sgwilym sgwilym commented Jan 10, 2022

What's the problem you solved?

In #129 it was reported that

  • code coverage wasn't working due to noble/ed22519 requiring an import map and the deno coverage command not supporting import maps
  • The import for noble/ed25519 was then changed to https://esm.sh/@noble/[email protected] by @cinnamon-bun.
  • Tests suddenly started failing.

Eventually @cinnamon-bun inlined this library in #130.

I took a closer look at all of this today, and think this is what happened:

Tests began to sporadically fail due to the importing via esm.sh, which automatically transforms NPM packages to be ESM-friendly. This is why we couldn't follow why the code wasn't working: none of us were looking at the transformed code esm.sh was serving. This transformed code had some kind of async leak in it which caused tests to fail.

The inlined version of noble/ed25519 introduced in #130 was still a bit problematic because it imported deno/std/node/crypto (basically a giant crypto polyfill) and still used it even in non-Node environments. This made the web bundle about 1mb in size.

Even without the use of an import map, our coverage commands were still failing due to our usage of window.indexedDB and deno coverage not supporting the --config flag which would allow us to import the appropriate types for IndexedDB.

What solution are you recommending?

With this solution:

  • There are no sporadic test failures
  • Deno and the browser use web crypto
  • Node uses Node's built-in crypto module
  • The coverage commands work
  • We excise the need for an import map
  • We excise the need for a deno.json config file
  • Web bundles go from ~1mb to ~300kb in size

Here's how it's achieved:

  • I have made a web-crypto only fork of noble/ed25519 at https://github.com/sgwilym/noble-ed25519. I'm going to work to merge some changes up to the original repo so that we don't need to vendor it.
    • This obviates the need of an import-map.
    • This fixes test failures we saw in Github workflows.
    • This makes web bundles a third of the size.
  • I have re-added inline types for IndexedDB. Previously I was using some compiler options in deno.json to get these types, but I have learned that all downstream users would need a similar deno.json too.
    • This obviates the need of deno.json
    • And also means that deno coverage, which does not support the --config flag, now works again.
  • I have added a mapping for noble/ed25519 to scripts/build_npm.ts so that the NPM build uses the normal NPM build (which includes support for Node's crypto).

Closes #129
Maybe also #125, as creating a web bundle works locally.

@cinnamon-bun cinnamon-bun merged commit 902a49f into esm-first Jan 10, 2022
@cinnamon-bun cinnamon-bun deleted the noble-investigation branch January 10, 2022 18:00
@cinnamon-bun
Copy link
Member

Wow great work!

My only concern is that the import from github is not versioned -- probably we should import from a specific commit. But I'm merging this now, will deal with that later.

@cinnamon-bun
Copy link
Member

PR to pin the import version from github: #134

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants