Skip to content
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

fix: add support for node16 module resolution #52

Closed
wants to merge 1 commit into from

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Jun 10, 2022

@Gozala
Copy link
Contributor Author

Gozala commented Jun 10, 2022

😩 I have no idea how this managed to break things

@rvagg
Copy link
Member

rvagg commented Jun 14, 2022

(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
Copy link
Member

rvagg commented Jun 14, 2022

@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
Copy link
Contributor Author

Gozala commented Jun 14, 2022

@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.

  • In practice this prevents packages that depends on dag-pb from enabling node16 (that is how I run into this)
  • There is high motivation to enable node16 mode as that is only way to import by package name inside the package without resorting to brittle path substitution hacks as we do here
  • module: node16 follows actual node module resolution logic, which is less error prone than path mapping with typeVersions (more on that later).

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.

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
Copy link
Contributor Author

Gozala commented Jun 14, 2022

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
Copy link
Member

rvagg commented Jun 14, 2022

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
Copy link
Member

achingbrain commented Jun 14, 2022

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
Copy link
Member

rvagg commented Jun 15, 2022

ipld/ipld#224

@BigLep
Copy link

BigLep commented Jul 19, 2022

Blocked until ipld/ipld#224 is completed

@rvagg
Copy link
Member

rvagg commented Nov 29, 2022

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

@rvagg rvagg closed this Nov 29, 2022
@rvagg rvagg deleted the fix/node16-ts-resolution branch November 29, 2022 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

4 participants