JavaScript IPLD: ESM migration tracking issue

[rvagg]

Ref: ipld/js-dag-pb#52

Background

The "new" IPLD stack was designed as a dual-publish - it's written in ESM, with TypeScript annotations and then compiled to CJS & ESM in a dist/ directory and the types are checked and compiled into dist/types/ directory (there may be a case where it's just an index.d.ts, not a types/ directory) and then npm publish is performed from the dist/ directory only.

So, publish: https://unpkg.com/browse/@ipld/dag-cbor@7.0.2/, doesn't match source: https://github.com/ipld/js-dag-cbor, but it can then be consumed with either require() or import and those will then use the correct forms--import giving you proper ESM so you get the proper static tree that you can shake etc.

Compiling uses ipjs which uses rollup under the hood to compile the code and a bunch of fairly static rules to decide how to create the new dist/ output - including parent package.json, its export map and cjs/ & esm/ directories and their package.json files (for "type": ".." switching).

But, ipjs locks us into a very singular pattern that has been difficult to evolve, and the latest problem @ ipld/js-dag-pb#52 is that we can't add a new entry to the package.json's "exports" map because ipjs has opinions about what should be in there so it knows what to compile and what to output. We could fix ipjs (again), or we could drop the compile step entirely and go with ESM-only.

The JS ecosystem has been rapidly embracing ESM, with many popular packages going ESM-only (most notably all of sindresorhus's many and very popular packages are now ESM only, node-fetch is ESM-only, it's getting hard to do pure CJS and rely on current ecosystem packages without awkward breakage). @achingbrain has been converting the libp2p and ipfs JS packages to publish ESM only (as well as fully embracing TypeScript authoring in the process). So .. it's time.

TODO

1. We need a repeatable pattern to implement in these repos, there already is a lot of consistency but there's also little bit of variation and switching to ESM could introduce more if we're not careful.
   a. A stretch-goal here might be to extract parts of the pattern to unified-ci - mainly the Actions bits.
2. Build step: we still need to compile the types before publishing, so there should probably still be an "build" script that does this. In https://github.com/rvagg/js-bitcoin-block/blob/e91b70c7a38183e9314cb2cb55ee973396724f35/package.json#L24-L31, which doesn't dual-publish, I had a simple "build" that was included with "lint" on every "test", maybe that's appropriate? An addition to GitHub Actions that checks git state to make sure that post-"build" doesn't produce different files might be good too.
3. We need to figure out type definition export settings - currently we have "types" in some packages (but not all) and "typesVersions" in most (all?). fix: add support for node16 module resolution js-dag-pb#52 introduces a "types" export field as well. Some packages have many exports and some have a single export, hence the difference. "typesVersions" with "*" mapping was originally introduced to deal with a TS bug (history of this will probably be in js-multiformats somewhere), we may be able to remove that? Moving forward with 3 separate ways to indicate type definition files is not going to be happy.

The following repos are the most important to make the conversion in:

• js-multiformats: v10.0.0 tracking multiformats/js-multiformats#199
• js-dag-cbor
• js-dag-json
• js-dag-pb: feat!: publish as esm-only js-dag-pb#56
• js-car

Additional repos that are actively maintained that can be converted with lower priority:

• js-murmur3
• js-sha3
• js-blake2
• js-printify
• js-ipld-schema (NOTE: this repo needs more work than just ESM, it shouldn't publish until it's back in sync with the fixtures .. so this conversion could be deferred)

Other repos that are currently in my GitHub could probably be included too (I could move these to the ipld org at the same time): iamap, js-ipld-hashmap, js-ipld-garbage, js-ipld-schema-validator

[achingbrain]

Could I suggest here, if it's not too controversial, maybe using aegir? It solves pretty much all of the problems above and works with Unified CI already.

More than happy to remove functionality (and deps) from it, if the install time is still an issue, but it seems like there's no reason to reinvent the wheel and all the projects can benefit from some collaboration on the tooling.

[rvagg]

I'm still allergic to kitchen-sink packages like aegir, but with all the tooling we're accumulating in these repos it's becoming a hard case to maintain since the devDependency is getting a bit out of hand as it is. My main objection is the contribution hostility for outsiders faced with weird custom tooling ... but we kind of already have that and we don't exactly have people lining up to contribute! So, no, I won't stand in the way of trying to use aegir for this the person(s) tackling this decides it's a more straightforward way to go. The case for using it to get these into unified-ci is pretty strong too.

[RangerMauve]

Some thoughts/questions:

I think it'd be good to ditch commonjs and any build steps if we can.

Do we need to publish the type definitions in separate files when we already have the annotations? Is tsc unable to parse the types out of dependencies with it?

It feels like it should be possible to have all the code be ESM-only, with typescript annotations in the source files as is. Then from JS (in a recent node version or with rollup) you could use import on the js files directly, and use the same import semantics from TypeScript and have the compiler pull the annotations out of the JS file itself. 🤷

Since it seems like we're comfy with having potentially breaking changes, it might be a good time to rip out any non-essential tooling.

[rvagg]

Good question @RangerMauve, I'm not sure TypeScript is quite that intelligent but maybe @Gozala can answer this question.

[Gozala]

Do we need to publish the type definitions in separate files when we already have the annotations? Is tsc unable to parse the types out of dependencies with it?

I’m afraid TS just won’t do it 😭 I really wish there was a way to opt in though.

That said I find compiling just types not to be nearly as painful as transpiling JS, as You can copy & paste code into node repl, import etc without running compilation. Most of type generation usually happens in CI

[RangerMauve]

The whole "can't import jsDoc types" thing might be related to this: microsoft/TypeScript#33136

Doing some digging and testing today.

[RangerMauve]

So, we can get this working if people add to their includes section in their tsconfig.json.

E.g. if we have dag-json only using JSDoc for types, users would need to do the following:

tsconfig.json

{
  "includes": [
    "node_modules/dag-json/**/*.js"
  ]
}

Is this something we would want to subject users to for the sake of getting rid of tooling? (I'm guessing no, but wanted to put it out there anyway)

Related to: microsoft/TypeScript#29824

[rvagg]

I think we're aiming for making users' lives simple here, so making them add that kind of config isn't going to be awesome UX!

We're still going to have tsc in the toolchain, it's been very useful as a lint-like tool for our codebases even when we don't use TypeScript; so given that it's not going away, what we're dealing with is an output-typedefs step, which I don't think is a big deal. It's a bit like a lint but it sometimes involves in some changed files to commit. We just need to make sure that what we publish has the right types—I'm +1 on checking in the typedefs in a types/ directory if we do this, but since we auto-publish anyway we have the option of .gitignoring that but including it on the publish, where the publish involves a build step. Either way is fine as long as we have consistency and don't run the risk of having typedefs out of sync I think.

[Gozala]

Is this something we would want to subject users to for the sake of getting rid of tooling? (I'm guessing no, but wanted to put it out there anyway)

This additionally implies that version of typescript on user device may produce different results and sometimes even error if library uses newer version with more features than client.

Overall I think generating those types on publish is a better compromise than subject users to configuring ts differently which I'm almost certain would be a big can of worms.

[RangerMauve]

I've been doing some more playing around with how we could go about ESM/TS stuff. You can see a mockup of what it would look like to import the dependency in this folder: https://github.com/RangerMauve/ts-dependency-test

If we're okay with going full-ESM and having some breaking changes (major version release for affected packages), I'd like to propose the following:

• Remove the ipjs dependency
• Stick with TS annotations
• Generate d.ts files for all the js files using regular tsc with emitDeclarationOnly: true in the tsconfig.json
• Add those d.ts files to .gitignore so they aren't commited (or we can commit them so that the type changes will be more visible. 🤷)
• Make sure the d.ts files do get published on NPM
• Remove the types, typesVersions, and types fields from package.json
• Remove mentions of cjs from docs
• Have the exports field point to the js files (I think that's the current behavior anyway)

With that in place:

• ESM based JavaScript will be able to import whatever files are in the exports of the package.json
• TypeScript will also follow the exports field to resolve the JS files, and will then find the d.ts files for the JS file automatically
• The overall folder structure will be more simple to inspect since the source will be the actual source
• We can get rid of extra tooling and stick to using raw tsc for generating the types for when we publish

How does this sound?
Should I also do some testing with Deno before working on the conversion?

[RangerMauve]

Just did a test with deno, and all I needed to add was to make sure all the exports keys and values in package.json started with ./

[Gozala]

Remove the types, typesVersions, and types fields from package.json

This would break bunch of things. Only new TS supports exports field meaning libraries that aren't on newest one will break. Additionally even if libraries want to upgrade to newest, but one of their dependency has not, they'll be in a position of having to either upgrade and ignore that dependency or stick to old version and ignore this one.

[Gozala]

Other that that what I pointed above plan sounds really good to me. In other words I'd just keep types and typeVersions until exports map becomes default in TS world after which it can be safely dropped.

[RangerMauve]

Would you mind enumerating which TypeScript versions must be supported for this effort? I could make sure to test against it.

[rvagg]

which TypeScript versions

@Gozala will have stronger opinions about this than me but I'm fine with pushing fairly hard on this to be "latest version from when we publish" and since we'll be doing semver-major for all of this then we get to signal any potential breakage. I believe that the TS community moves fairly fast with version upgrades so this shouldn't be a major problem? Or maybe this is one of those LTS/enterprise things where we have to care about a specific line of versions? The trick might also be all of the ways in which TS projects can configure what they support and how they support it in tsconfig.json.

Some other notes coming out of our discussion on Zoom just now:

• js-multiformats commit history is likely the place to find reasoning for the weird typesVersions usage, we've had to deal with multiple TS bugs and issues in there and it's been a pain in the backside. I'd love for it all to go away.
• Some projects have a test/ts-use/ TS project in them, I'd love to see this rolled out further across the projects and expanded too to test more of the types (it's pretty minimal where it exists) - not a strict requirement but it does provide a level of assurance that we're getting things right.
• It seems like we're leaning toward not checking in types to the repo but only doing it on publish, I'm fine with this as long as (a) we're type checking at test time (as a lint it's been very helpful) and (b) we're absolutely sure there's consistency between what's tested & committed and what's published.
• If you want to start simple, maybe bite off one of the multihash packages - sha3, murmur, blake2. They're relatively simple setups. In terms of complexity, js-dag-cbor and js-dag-json are the next most simple, then js-dag-pb would be next but it has multuple exports. js-car has a bunch of exports and gets more complicated and js-multiformats is our most complex by far and also the most critical to get right (it's where folks will squeal the loudest if we mess up).
• in terms of aegir - not a critical component, could be left off or could be deferred to later. But as Alex points out, they've solved a bunch of these issues over there already with a consistent pattern and it has some flexibility to adapt to what we want here I think. It's also wired up to the unified-ci (https://github.com/protocol/.github) so that there's one place to update GH Actions .. whereas we have to do that in each repo. I did want to get an IPLD version of that set up so our repos have one place to update, but I haven't even started doing that. It might not be hard to have a second JS setup in unified-ci for us that we can roll out with a single update using the patterns already established in the IPLD & multiformats repos (also some in filecoin-shipyard). But, this probably should be considered a separate issue to minimise scope.

Other than the types & typesVersions concerns discussed above, your list is in line with my expectations I think @RangerMauve.

The biggest thing I'm going to miss with all of this is the ability to require from the Node.js REPL, currently I can easily require('/path/to/js-dag-cbor/dist') to get dag-cbor .. I used it almost daily. Pulling in ESM on the REPL is more of a pain... I hope we get top-level await or something soon to deal with that.

[Gozala]

Would you mind enumerating which TypeScript versions must be supported for this effort? I could make sure to test against it.

@RangerMauve I can't really speak to that. All I can say is that our team wanted to opt-in into export maps based resolution, which if I'm not mistaken, is not the default in TS. It can be enabled by setting "moduleResolution": "nodenext". However with that setting TS failed to resolve majority of dependencies that use typeVersions or types in package.json.

Given that this is opt-in and causes problems with existing dependencies we had to disable. Which is why I'm hesitant to just drop everything other than export maps, changes are many dependents will:

1. Start getting issues and not know that they need to use non default module resolution.
2. Even if they do know to change moduleResolution on in their config, they may not be able to do so because other dependencies will become unresolveable.

More simply currently either all you dependencies need to opt-in into new moduleResolution or you won't be able to resolve modules from ones that don't.

Given this I think it is reasonable to keep typeVersions and types fields around until "nodenext" module resolution is default in TS. Hopefully by then most packages will add type to their "export" maps so thing would just work.

[Gozala]

• js-multiformats commit history is likely the place to find reasoning for the weird typesVersions usage, we've had to deal with multiple TS bugs and issues in there and it's been a pain in the backside. I'd love for it all to go away.

In most of our packages types info is fairly straight forward:

"types": "./dist/src/lib.d.ts",
"typesVersions": {
    "*": {
      "*": [
        "dist/src/*"
      ],
   }
}

And TS does all the .d.ts generation via following settings:

"declarationMap": true,
"emitDeclarationOnly": true,
"outDir": "./dist/"

js-multiformats is complex because ipjs copies stuff into build directory and which makes layout on install different from layout in dev.

[Gozala]

Some projects have a test/ts-use/ TS project in them, I'd love to see this rolled out further across the projects and expanded too to test more of the types (it's pretty minimal where it exists) - not a strict requirement but it does provide a level of assurance that we're getting things right.

To be honest only place I felt we needed something like this is in js-multiformats and that again was stemmed from the fact that directory layout on install is different from layout on in dev / tests. Which meant that when package installed ts defs were not in right place and that was never caught.

I have not run into similar issue any any other package and mostly we sprinkle @ts-expect-error-s in places where we want to verify TS catches missmatched types e.g. https://github.com/web3-storage/ucanto/search?q=ts-expect-error&type=code

[Gozala]

It seems like we're leaning toward not checking in types to the repo but only doing it on publish, I'm fine with this as long as (a) we're type checking at test time (as a lint it's been very helpful) and (b) we're absolutely sure there's consistency between what's tested & committed and what's published.

We use this flow across many packages as well, we have a github workflow that reports type missmatches inline in the diff view https://github.com/gozala/typescript-error-reporter-action. We find it to be much better then TS error dump which it produces if your CI workflow just does tsc --build. It also uses same ts and tsconfig you specify so results are consistent.

Our npm publish workflow also runs tsc --build and generated types end up in dist/. I think it had been working pretty well.

[Gozala]

The biggest thing I'm going to miss with all of this is the ability to require from the Node.js REPL, currently I can easily require('/path/to/js-dag-cbor/dist') to get dag-cbor .. I used it almost daily. Pulling in ESM on the REPL is more of a pain... I hope we get top-level await or something soon to deal with that.

@rvagg I switched to await import('./src/lib.js') instead which is bit more typing, but other than than provides same REPL experience. I also have this handy alias

alias node-repl="node -i -r $HOME/.repl.js"

and ~/.repl.js with following code:

const main = async () => {
  try {
    const config = `${process.cwd()}/.repl.js`
    Object.assign(globalThis, await import(config))
    console.log(`Config loaded from ${config}\n`)
  } catch (error) {
    console.log(error)
  }
}

main()

That way I sprinkle .repl.js files in repos I use repl that re-export things I care about in that context so when I run node-repl I get all the common imports preset in globals.

[RangerMauve]

@rvagg @BigLep Any chance I could be added as a collaborator to https://github.com/multiformats/js-multiformats or the multiformats org in general? I'd like to make a branch for the ESM migration right in the repo if possible. 😁

[RangerMauve]

I can just make a fork on my own account in the meantime though.

[vmx]

I've added @RangerMauve via multiformats/github-mgmt#30, which is already merged.

[BigLep]

2022-07-19 conversation: main open thing was whether to use Aegir or Unified CI.

[rvagg]

I've linked back to here from all of the dependabot PRs for multiformats@10.0.0 that are breaking, so the list is above ^.

[BigLep]

2022-11-15 IPLD triage call: @RangerMauve will likely pick this up after the first IPLD WG meeting.