-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JavaScript IPLD: ESM migration tracking issue #224
Comments
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. |
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. |
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 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 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. |
Good question @RangerMauve, I'm not sure TypeScript is quite that intelligent but maybe @Gozala can answer this question. |
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 |
The whole "can't import jsDoc types" thing might be related to this: microsoft/TypeScript#33136 Doing some digging and testing today. |
So, we can get this working if people add to their E.g. if we have 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 |
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 |
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. |
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:
With that in place:
How does this sound? |
Just did a test with deno, and all I needed to add was to make sure all the |
This would break bunch of things. Only new TS supports |
Other that that what I pointed above plan sounds really good to me. In other words I'd just keep |
Would you mind enumerating which TypeScript versions must be supported for this effort? I could make sure to test against it. |
@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:
Other than the The biggest thing I'm going to miss with all of this is the ability to |
@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 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:
More simply currently either all you dependencies need to opt-in into new Given this I think it is reasonable to keep |
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. |
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 |
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 Our npm publish workflow also runs |
@rvagg I switched to
and 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 |
@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. 😁 |
I can just make a fork on my own account in the meantime though. |
As requested at ipld/ipld#224 (comment), add @RangerMauve as collaborator to js-multiformats
I've linked back to here from all of the dependabot PRs for [email protected] that are breaking, so the list is above ^. |
Following on from the conversation in ipld/ipld#224 this converts this repo to use the latest aegir with Unified CI. 1. Remove all build related dev deps apart from `aegir` 1. Run the `npx aegir check-project` command to update project config 1. Remove non-Unified-CI github actions 1. Update imports to import from `src/index.js` instead of `@ipld/dag-pb` 1. Rename `test/*.js` to `test/*.spec.js` so aegir can find them 1. Update `tsconfig.json` to extend config from `aegir` 1. Remove `"main"` and other unused fields from `package.json` 1. Use `chai` from `aegir` pre-configured with plugins we use 1. Fixes everything the linter complains about This will need a follow up PR to `protocol/.github` to add this repo to the Unified CI [config file](https://github.com/protocol/.github/blob/master/configs/js.json) so it'll get automated config updates in the future. Apologies that this PR is so noisy, most of it is from the `check-project` command
Following on from the conversation in ipld/ipld#224 this converts this repo to use the latest aegir with Unified CI. 1. Remove all build related dev deps apart from `aegir` 1. Run the `npx aegir check-project` command to update project config 1. Remove non-Unified-CI github actions 1. Update imports to import from `src/index.js` instead of `@ipld/dag-pb` 1. Rename `test/*.js` to `test/*.spec.js` so aegir can find them 1. Update `tsconfig.json` to extend config from `aegir` 1. Remove `"main"` and other unused fields from `package.json` 1. Use `chai` from `aegir` pre-configured with plugins we use 1. Fixes everything the linter complains about This will need a follow up PR to `protocol/.github` to add this repo to the Unified CI [config file](https://github.com/protocol/.github/blob/master/configs/js.json) so it'll get automated config updates in the future. Apologies that this PR is so noisy, most of it is from the `check-project` command
Following on from the conversation in ipld/ipld#224 this converts this repo to use the latest aegir with Unified CI. 1. Remove all build related dev deps apart from `aegir` 1. Run the `npx aegir check-project` command to update project config 1. Remove non-Unified-CI github actions 1. Moved source files into `src` 1. Rename `test/*.js` to `test/*.spec.js` so aegir can find them 1. Update `tsconfig.json` to extend config from `aegir` 1. Remove `"main"` and other unused fields from `package.json` 1. Use `chai` from `aegir` pre-configured with plugins we use 1. Fixes everything the linter complains about This will need a follow up PR to `protocol/.github` to add this repo to the Unified CI [config file](https://github.com/protocol/.github/blob/master/configs/js.json) so it'll get automated config updates in the future. Apologies that this PR is so noisy, most of it is from the `check-project` command
Following on from the conversation in ipld/ipld#224 this converts this repo to use the latest aegir with Unified CI. 1. Remove all build related dev deps apart from `aegir` 1. Run the `npx aegir check-project` command to update project config 1. Remove non-Unified-CI github actions 1. Moved source files into `src` 1. Rename `test/*.js` to `test/*.spec.js` so aegir can find them 1. Update `tsconfig.json` to extend config from `aegir` 1. Remove `"main"` and other unused fields from `package.json` 1. Use `chai` from `aegir` pre-configured with plugins we use 1. Fixes everything the linter complains about This will need a follow up PR to `protocol/.github` to add this repo to the Unified CI [config file](https://github.com/protocol/.github/blob/master/configs/js.json) so it'll get automated config updates in the future. Apologies that this PR is so noisy, most of it is from the `check-project` command
Following on from the conversation in ipld/ipld#224 this converts this repo to use the latest aegir with Unified CI. 1. Remove all dev deps apart from `aegir` 1. Run the `npx aegir check-project` command to update project config 1. Remove non-Unified-CI github actions 1. Update imports to import from `src/index.js` instead of `@multiformats/murmur3` 1. Rename `test/*.js` to `test/*.spec.js` so aegir can find them 1. Update `tsconfig.json` to extend config from `aegir` 1. Remove `"main"` and other unused fields from `package.json` 1. Use `chai` from `aegir` pre-configured with plugins we use 1. Fixes everything the linter complains about This will need a follow up PR to `protocol/.github` to add this repo to the Unified CI [config file](https://github.com/protocol/.github/blob/master/configs/js.json) so it'll get automated config updates in the future. Apologies that this PR is so noisy, most of it is from the `check-project` command
Following on from the conversation in ipld/ipld#224 this converts this repo to use the latest aegir with Unified CI. 1. Remove all dev deps apart from `aegir` 1. Run the `npx aegir check-project` command to update project config 1. Remove non-Unified-CI github actions 1. Update imports to import from `src/index.js` instead of `ipld-garbage` 1. Rename `test/*.js` to `test/*.spec.js` so aegir can find them 1. Update `tsconfig.json` to extend config from `aegir` 1. Remove `"main"` and other unused fields from `package.json` 1. Use `chai` from `aegir` pre-configured with plugins we use 1. Fixes everything the linter complains about 1. Upgrades multiformats to 10.x.x This will need a follow up PR to `protocol/.github` to add this repo to the Unified CI [config file](https://github.com/protocol/.github/blob/master/configs/js.json) so it'll get automated config updates in the future. Apologies that this PR is so noisy, most of it is from the `check-project` command
Following on from the conversation in ipld/ipld#224 this converts this repo to use the latest aegir with Unified CI. 1. Renamed repo to `@ipld/garbage` 1. Remove all dev deps apart from `aegir` 1. Run the `npx aegir check-project` command to update project config 1. Remove non-Unified-CI github actions 1. Update imports to import from `src/index.js` instead of `ipld-garbage` 1. Rename `test/*.js` to `test/*.spec.js` so aegir can find them 1. Update `tsconfig.json` to extend config from `aegir` 1. Remove `"main"` and other unused fields from `package.json` 1. Use `chai` from `aegir` pre-configured with plugins we use 1. Fixes everything the linter complains about 1. Upgrades multiformats to 10.x.x
Following on from the conversation in ipld/ipld#224 this converts this repo to use the latest aegir with Unified CI. 1. Remove all build related dev deps apart from `aegir` 1. Run the `npx aegir check-project` command to update project config 1. Remove non-Unified-CI github actions 1. Rename `test/*.js` to `test/*.spec.js` so aegir can find them 1. Update `tsconfig.json` to extend config from `aegir` 1. Remove `"main"` and other unused fields from `package.json` 1. Use `chai` from `aegir` pre-configured with plugins we use 1. Fixes everything the linter complains about 1. Update `multiformats` to 10.x.x
Following on from the conversation in ipld/ipld#224 this converts this repo to use the latest aegir with Unified CI. 1. Remove all build related dev deps apart from `aegir` 1. Run the `npx aegir check-project` command to update project config 1. Remove non-Unified-CI github actions 1. Update imports to import from `src/index.js` instead of `@ipld/dag-pb` 1. Rename `test/*.js` to `test/*.spec.js` so aegir can find them 1. Update `tsconfig.json` to extend config from `aegir` 1. Remove `"main"` and other unused fields from `package.json` 1. Use `chai` from `aegir` pre-configured with plugins we use 1. Fixes everything the linter complains about 1. Updates `multiformats` to 10.x.x Co-authored-by: Rod Vagg <[email protected]>
Following on from the conversation in ipld/ipld#224 this converts this repo to use the latest aegir with Unified CI. 1. Remove all build related dev deps apart from `aegir` 1. Run the `npx aegir check-project` command to update project config 1. Remove non-Unified-CI github actions 1. Moved source files into `src` 1. Rename `test/*.js` to `test/*.spec.js` so aegir can find them 1. Update `tsconfig.json` to extend config from `aegir` 1. Remove `"main"` and other unused fields from `package.json` 1. Use `chai` from `aegir` pre-configured with plugins we use 1. Fixes everything the linter complains about 1. Updates `multiformats` to 10.x.x BREAKING CHANGE: this module is now ESM-only
2022-11-15 IPLD triage call: @RangerMauve will likely pick this up after the first IPLD WG meeting. |
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/[email protected]/, doesn't match source: https://github.com/ipld/js-dag-cbor, but it can then be consumed with either
require()
orimport
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
a. A stretch-goal here might be to extract parts of the pattern to unified-ci - mainly the Actions bits.
"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."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:
Additional repos that are actively maintained that can be converted with lower priority:
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
The text was updated successfully, but these errors were encountered: