v10.0.0 tracking

[rvagg]

Includes both #197 and #196. Will comment in both of those what I've done.

I've also applied some additional linting to get #162 to jive with #167 and have also made js-test-and-release.yml run on: [push, pull_request] for now, that'll get updated soon enough with https://github.com/protocol/.github/blob/master/templates/.github/workflows/js-test-and-release.yml (although I'm not sure why it's so restrictive).

Unfortunately for some reason, bringing both of the big PRs together makes aegir / pw-test fail to see the "browser" field for the hashes/sha2 export and it borks on not being able to find "crypto". I haven't figured out what's going on with that and I'm not sure what would have changed to start triggering that when it isn't happening in the esm-migration branch. You can see what's changed here: https://github.com/multiformats/js-multiformats/compare/proposal/v10.0.0..esm-migration

Open remaining items:

• fix: remove use of Object.defineProperties in CID class #202

Items not in scope:

• CIDs as interfaces #203

Items we'll be able to address afterwards as a result:

• Add js-multiformats to unified-ci protocol/.github#382
• fix: add support for node16 module resolution ipld/js-dag-pb#52
• feat: use new multiformats CID interface and exports ipld/js-bitcoin#7

[rvagg]

Actions isn't showing the failing tests, it looks like test:browser isn't enough to trigger browser tests with the js-test-and-maybe-release.yml runners: https://github.com/multiformats/js-multiformats/runs/8201994927, so we'll have to address that.

[rvagg]

pinging @achingbrain who might be able to see what's going on with the browser tests

✘ [ERROR] Could not resolve "crypto"

    src/hashes/sha2.js:3:19:
      3 │ import crypto from 'crypto'
        ╵                    ~~~~~~~~

  The package "crypto" wasn't found on the file system but is built into node. Are you trying to
  bundle for node? You can use "platform: 'node'" to do that, which will remove this error.

✖ Running tests failed.
Error: Build failed with 1 error:
src/hashes/sha2.js:3:19: ERROR: Could not resolve "crypto"

that's supposed to be addressed with the browser export for ./hashes/sha2 but now it doesn't work, even though it did in #196.

[achingbrain]

and have also made js-test-and-release.yml run on: [push, pull_request] for now

Any changes to this file will get overwritten by the Unified CI bot, you should PR the source file instead. The change looks fine though, I don't think there's any need for it to be so restrictive.

what's going on with the browser tests

Just looking into this. Normally you'd use the "browser" field in package.json to specify browser overrides for internal files but that doesn't seem to be specified in this project, I'm guessing because IPJS would previously have overwritten it?

I think it might be related to these sorts of imports in the tests:

import { sha256 } from 'multiformats/hashes/sha2'

Before I go any futher in investigating is there any reason the import isn't using relative paths instead of trying to resolve multiformats from node_modules?

[RangerMauve]

Just looking into this. Normally you'd use the "browser" field in package.json to specify browser overrides for internal files but that doesn't seem to be specified in this project

This project is relying on ESM Exports in the package.json and using the browser field for the target environment. What is Aegir using for the bundling before it runs tests in the browser? Does it maybe need to have it's Webpack version updated?

[achingbrain]

What is Aegir using for the bundling before it runs tests in the browser?

It uses esbuild for bundling.

[RangerMauve]

Weird. Esbuild should be detecting the exposts field as of 0.9.0 and aegir is using a much newer one. evanw/esbuild#187

[achingbrain]

Yeah, the whole thing is a bit weird to be honest - this module depends on @ipld/dag-pb which depends on multiformats@9.8.1, so it gets hoisted to the node_modules root for me.

The test then has:

import { sha256 } from 'multiformats/hashes/sha2'

..so it's going to resolve to node_modules/multiformats/... at which point the exports map will be consulted, the "browser" field found and node_modules/multiformats/src/hashes/sha2-browser.js returned.

But, we're (accidentally?) importing from the hoisted node_modules/multiformats and not ../src/hashes/sha2.js as I think is the intention here.

If it was:

import { sha256 } from '../src/hashes/sha2.js'

with an override in the "browser" field (not "exports" since this is an internal import path) it should work?

[RangerMauve]

Aha! Good digging. I think it might actually be related to the package.json in the ts-use folder trying to load multiformats from the dist folder which no longer exists in this version

[RangerMauve]

I think we need to tests to import the ts-use module as is rather than importing the main.ts file from it.

[RangerMauve]

Err, the error defs isn't due to the ts-use folder. 🤔

[RangerMauve]

Did some digging in the build, and this is what's happening when it tries to import sha2 from the "test" directory:

⬥ [VERBOSE] Resolving import "multiformats/hashes/sha2" in directory "/home/mauve/programming/js-multiformats/test" of type "import-statement"

  Read 11 entries for directory "/home/mauve/programming/js-multiformats/test"
  No "browser" map found in directory "/home/mauve/programming/js-multiformats/test"
  Searching for "multiformats/hashes/sha2" in "node_modules" directories starting from "/home/mauve/programming/js-multiformats/test"
    Matching "multiformats/hashes/sha2" against "paths" in "/home/mauve/programming/js-multiformats/tsconfig.json"
      Using "/home/mauve/programming/js-multiformats" as "baseURL"
      Found a fuzzy match for "multiformats/*" in "paths"
      Attempting to load "/home/mauve/programming/js-multiformats/src/hashes/sha2.js" as a file
        Checking for file "sha2.js"
        Found file "sha2.js"
  Read 15 entries for directory "/home/mauve/programming/js-multiformats/src"
  Read 7 entries for directory "/home/mauve/programming/js-multiformats/src/hashes"
  This import is under the effect of "/home/mauve/programming/js-multiformats/tsconfig.json"
  Primary path is "/home/mauve/programming/js-multiformats/src/hashes/sha2.js" in namespace "file"

⬥ [VERBOSE] Resolving import "multiformats/hashes/sha2" in directory "/home/mauve/programming/js-multiformats/test" of type "import-statement"

  Read 11 entries for directory "/home/mauve/programming/js-multiformats/test"
  No "browser" map found in directory "/home/mauve/programming/js-multiformats/test"
  Searching for "multiformats/hashes/sha2" in "node_modules" directories starting from "/home/mauve/programming/js-multiformats/test"
    Matching "multiformats/hashes/sha2" against "paths" in "/home/mauve/programming/js-multiformats/tsconfig.json"
      Using "/home/mauve/programming/js-multiformats" as "baseURL"
      Found a fuzzy match for "multiformats/*" in "paths"
      Attempting to load "/home/mauve/programming/js-multiformats/src/hashes/sha2.js" as a file
        Checking for file "sha2.js"
        Found file "sha2.js"
  Read 7 entries for directory "/home/mauve/programming/js-multiformats/src/hashes"
  This import is under the effect of "/home/mauve/programming/js-multiformats/tsconfig.json"
  Primary path is "/home/mauve/programming/js-multiformats/src/hashes/sha2.js" in namespace "file"

I think the issue is that the tests are trying to import from multiformats/ rather than ../ as @achingbrain alluded to

[RangerMauve]

Removing the "paths" section from tsconfig has yielded the following errors in the build:

test/test-block.spec.js:66:37 - error TS2322: Type 'string' is not assignable to type 'CID | Record<string, any>'.

66       assert.deepStrictEqual(ret, { value: 'skip' })
                                       ~~~~~

  node_modules/multiformats/types/src/block.d.ts:101:9
    101         value: CID;
                ~~~~~
    The expected type comes from property 'value' which is declared here on type '{ value: CID; remaining: string; } | { value: Record<string, any>; remaining?: undefined; }'

test/test-cid.spec.js:243:29 - error TS2339: Property 'link' does not exist on type 'CID'.

243       assert.equal(cid, cid.link())
                                ~~~~

test/test-link.spec.js:3:23 - error TS2307: Cannot find module 'multiformats/link' or its corresponding type declarations.

3 import * as Link from 'multiformats/link'
                        ~~~~~~~~~~~~~~~~~~~

test/test-link.spec.js:73:7 - error TS2578: Unused '@ts-expect-error' directive.

73       // @ts-expect-error - types can not be inferred
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

test/test-link.spec.js:95:9 - error TS2578: Unused '@ts-expect-error' directive.

95         // @ts-expect-error - version is 1 not 0
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[RangerMauve]

Err, nvm I think I messed stuff up with that. 😅

[RangerMauve]

Following from this discussion: ipld/js-dag-pb#56 (comment)

I don't think we can have the import from "multiformats/whatever" style format with ESM sadly. :/ I think we have to manually specify which file we want to import from, and that also means that browser support becomes harder.

[RangerMauve]

I think in order to get the raw specifiers thing to work, we'll need to treat the tests as a separate module similar to how ts-use does it.

[BigLep]

2022-09-06 triage conversation: going to discuss async what the next steps are.

[rvagg]

@RangerMauve I hope you don't mind but I'm going to force push your latest commit out of here since the tsconfig settings are to help get nicer local TS imports and they shouldn't impact external consumers.

So .. thanks for the input @achingbrain, you're right about the import picking up the one in node_modules, it's been so long since we set all this weird layout up that I'd forgotten how most of it worked. IIRC what we have is something like:

• We switched to the named imports, multiformats/... because that works in some bundlers (I think rollup and webpack allow this?), I think it was supposed to become a standard ESM thing (I thought it was already in Node.js but @RangerMauve corrected me today). But the main thing it let us do here was allow the rollup-based compile step to pick it all apart and put it back together again in both esm and cjs forms with appropriate relative links.
• ipjs does indeed set up the "browser" field, I've been hiding behind ipjs long enough that I'd forgotten this is still necessary for some bundlers and that we were simulating universal export map "browser" field support by automatically building it. But alas I guess we need both for some time still?
• The dag-pb import here is weird; it looks like we let it slip in as part of the traversal.js work (2bd8878) but it's really not necessary because it's only forming dag-pb shaped objects to traverse. So I've removed it as a dependency entirely.
• Removing dag-pb is nice, but it doesn't make multiformats go away as a transitive dependency, it comes back in now via uint8arrays (which uses it for the bases) through aegir, so it's probably going to be difficult to rid ourselves of it as a dependency unless we chase it away down there.

So my current push to this branch removes the named dependencies and uses relative ones, plus the "browser" field does seem to make esbuild happy.

I've also added in most of the build scripts @achingbrain suggested in ipld/js-dag-pb#56, although I've customised the "test" script because (a) I'd like linting in test, (b) it complains about coverage not being supported in webworkers (even though --cov isn't supplied—I guess it's a default? I've had to add --no-cov for all but the "test:chrome" script to make them work) (c) the electron test inclusion borks for me because I don't have my DISPLAY setup locally like it wants (I could set it up properly for it to work but I'd like to minimise hiccups like this for other contributors and electron tests can be done in CI) and (d) I wanted to get the ts-use tests in there.. Critiques of my approach?

Also, it's a bit sad to move away from named imports for tests, it was nice consuming the package from the tests according to what's exported. But that's going to be pretty hard without the ipjs toolchain doing the work for us. We could go with the ts-use style sub-package just for testing, but even then, we have multiformats in the node_modules, is there a possibility we'll still end up with confusion about which version it's importing even if we use "file:../../" as the import? And, is it worth it? 🤷

[rvagg]

I've published a 10.0.0-pre.1 with this as it is now (FYI @Gozala in case you want to try out the Link work on something)

[rvagg]

• pulled in fix: remove use of Object.defineProperties in CID class #202 with changes (see that thread)
• rebased to current master
• removed "main"

Ready to roll, 🤞 hoping that auto-release will give us a proper v10.0.0 on merge.

Any final comments or objections?

[achingbrain]

hoping that auto-release will give us a proper v10.0.0 on merge

It should do as long as the commit title has a ! (e.g. feat!: ...) and/or BREAKING CHANGE: ... is in the message.

[BigLep]

@rvagg : I think it's time to pull the trigger :)

[rvagg]

@BigLep yeah, I ran out of time yesterday to get the auto-release process tested - that it's not going to have problems with my pre-releases and that it's going to pick up a proper semver-major release for this. Thankfully I did bother testing because it wasn't going to semver-major this, I'd assumed that one of us had tagged at least one commit as breaking, but nope!

Manually merging and manipulating the commit history for a bit of squashing and rewriting of comments. Will push that to master and it should release a 10.0.0.