search

ipld / js-dag-pb

An implementation of the DAG-PB spec for JavaScript (for use with multiformats or @ipld/block)
Other
12 stars 5 forks source link

fix: add support for node16 module resolution #52

Closed Gozala closed 1 year ago

Gozala commented 2 years ago

Fixes module resolution with node16 ts configuration see: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#package-json-exports-imports-and-self-referencing

Gozala commented 2 years ago

😩 I have no idea how this managed to break things

rvagg commented 2 years ago

(node:1753) UnhandledPromiseRejectionWarning: Error: ENOENT: no such file or directory, open '/home/runner/work/js-dag-pb/js-dag-pb/types/src/index.d.ts'

An ipjs compile problem. It's using the exports list to figure out what to compile.

Either that needs to be fixed over there to ignore (and pass through) "types" as an export type, or we convert this to pure ESM and be done with it. I'm OK either way but we'll need to semver-major bump for an ESM switch.

rvagg commented 2 years ago

@Gozala on the original purpose of this - is there active breakage for not doing this here? Is this something we need to be doing across the board or just for this one because of the limited export scope? I'm not sure I understand what this is adding since we already have a "typesVersions" config and a "types" property. Is this third thing really needed? Can we remove stuff?

Gozala commented 2 years ago

@Gozala on the original purpose of this - is there active breakage for not doing this here? Is this something we need to be doing across the board or just for this one because of the limited export scope? I'm not sure I understand what this is adding since we already have a "typesVersions" config and a "types" property. Is this third thing really needed? Can we remove stuff?

There are several things here so I'll try to address them in bullet points below

is there active breakage for not doing this here?

If dependent package sets module: node16 in tsconfig TS starts looking at package exports and refuse to import modules not listed there.

Is this something we need to be doing across the board or just for this one because of the limited export scope?

I think we should start doing this everywhere we have exports in packages for reasons described above. While module: node16 is not a default yet, I suspect it will become one as things settle.

I'm not sure I understand what this is adding since we already have a "typesVersions" config and a "types" property.

  1. types field overlays main field of package.json. Just like in node main field is superseded by "exports": { ".": ... } when package's "type": "module" is set, same happens in TS if module: node16 is set. In other words TS will look in exports as opposed to types field AFIK.
  2. I consider typeVersions to be a hack that barely works.
    • It does not at all reflect node's module resolution, although often you can achieve desired effect.
    • typeVersion has several bugs we've encountered across bunch of our repos:
    • type: module packages may not export some modules, but typeVersions would happily let you import them.
    • With module: node16 enabled, TS does not let you import module unless path is in exports. I do not know if does not looks at typeVersions at all with node16, or performs those path substitutions afterwards.

Is this third thing really needed?

I don't know given the answers above, what do you think ?

Can we remove stuff?

It depends. If we're ok with saying either use node16 or simply import by package name than we could remove typeVersions. However given that some packages (like this one) may not work with node16 option it may place users in the position where they can't use neither node16 nor node (option) without breaking some package. So probably it would be best to have all 3 for some time and remove typeVersions once node16 mode becomes default.

It is also worth considering typeVersions a rough equivalent of cjs support. If we're comfortable dropping cjs we should do the same for typeVersions otherwise we should probably keep both around.

Gozala commented 2 years ago

Either that needs to be fixed over there to ignore (and pass through) "types" as an export type, or we convert this to pure ESM and be done with it. I'm OK either way but we'll need to semver-major bump for an ESM switch.

Would that imply dropping ipjs along with it ? If so, I honestly can't wait!

rvagg commented 2 years ago

Would that imply dropping ipjs along with it ? If so, I honestly can't wait!

Yeah, I think it's time. I'm over the dual publish and enough of the ecosystem has done it that I think we can get away with it with a semver-major bump.

@achingbrain any objections to moving toward ESM-only in ipld packages?

achingbrain commented 2 years ago

No objections from me - libp2p and ipfs are already ESM-only, forward to the glorious future!

If typescript now supports "types" in the "exports" map I guess that means we can also kill off all the typesVersions hacks too?

rvagg commented 2 years ago

https://github.com/ipld/ipld/issues/224

BigLep commented 2 years ago

Blocked until https://github.com/ipld/ipld/issues/224 is completed

rvagg commented 1 year ago

this is done now as part of the ESM upgrade, should be in the latest published version