paulmillr / noble-secp256k1

Fastest 4KB JS implementation of secp256k1 signatures and ECDH
https://paulmillr.com/noble
MIT License
757 stars 114 forks source link

Build ESM & CJS, expose via exports #32

Closed jacogr closed 2 years ago

jacogr commented 2 years ago

My attempt on following up on a similar PR in @noble/hashes to build this via ESM and CJS.

Additions:

While there are now 3 files in the root (index.ts, crypto.ts & cryptoBrowser.ts) I didn't move these to a src folder.

Any comments welcome as well as insights in to where I may have missed the ball.

paulmillr commented 2 years ago

How does this work with deno?

jacogr commented 2 years ago

I am not a deno expert :) Having said that, looking at it while doing this, since the mod.ts still imports index.ts (no changes/movements there), I believed it was still correct and would work without any additions.

I may be wrong in that is may base a breakdown where it requires @noble/secp256k1/crypto, not sure how to solve that one. (Would be solvable no doubt, just not familiar with the deno environment)

paulmillr commented 2 years ago

I don't think it's solvable. We need to test it, since it seems to be the main lib for secp on deno

jacogr commented 2 years ago

Just testing with rollup, we do have "browser": { "crypto": false } in package.json -

  1. Remove crypto.ts & cryptoBrowser.ts
  2. Remove crypto import and add the following to index.ts -
// Global symbol available in browsers only
declare const self: Record<string, any> | undefined;

import nodeCrypto from 'crypto';

export const crypto: { node?: any; web?: any } = {
  node: nodeCrypto,
  web: typeof self === 'object' && 'crypto' in self ? self.crypto : undefined,
};

The build output via npm build-release is as follows -

...
    const nodeCrypto = {};

    const crypto = {
        node: nodeCrypto,
        web: typeof self === 'object' && 'crypto' in self ? self.crypto : undefined,
    };
    ...

Since randombytes checks web first, it works as expected. So the crypto.ts looks like complete noise that should not be needed since have the crypto: false flag anyway.

PS: sorry for the close/re-open, really was a slip of the finger on the trackpad over here.

paulmillr commented 2 years ago

There are other bundlers besides rollup like Browserify, webpack. They need to be considered also.

paulmillr commented 2 years ago

Build-release only builds single file for standalone in-browser inclusion btw, it is not used when node.js-required.

jacogr commented 2 years ago

Yes, I do understand the role of build-release very well :) The "crypto": false in package.json is not a rollup-specific flag, it is a field for bundlers.

For webpack it ends up with an empty crypto require as well (CJS inputs) -

var __webpack_modules__ = ({
// my comment - the module id for @noble/secp256k1
656: (function ...
...
const crypto_1 = __importDefault(__webpack_require__(159));
... /// more secp256k1 code
const crypto = {
    node: crypto_1.default,
    web: typeof self === 'object' && 'crypto' in self ? self.crypto : undefined,
};
... // more secp256k1 code
/***/ }), // send 656

/***/ 159:
/***/ (() => {

/* (ignored) */

/***/ })
... // more webpack code, here for the require impl

Steps to replicate -

  1. new folder, cd folder
  2. yarn init
  3. yarn add webpack webpack-cli
  4. dummy index.htm using index.js as script source
  5. index.js file just importing util and executing randombytes on it (logging the output)
  6. replace secp256k1 contents under node_modules with the ones from this PR (along with updated package.json)
  7. run webpack (I did add a small webpack.config.js file to set optimization false for readable outputs)

EDIT: gist here with actual outputs - https://gist.github.com/jacogr/7691c4aa8c94c1c2faf680cca56bcee9 (empty crypto import on these lines - https://gist.github.com/jacogr/7691c4aa8c94c1c2faf680cca56bcee9#file-webpack-secp256k1-L993-L998)

For browserify, the browser filed is also explicitly mentioned in the handbook for ignores, e.g. https://github.com/browserify/browserify-handbook#browser-field (last example containing "winston": false)

paulmillr commented 2 years ago

Could you resolve conflicts?

paulmillr commented 2 years ago

@jacogr the tests are failing