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

export map means node always resolves CJS #7418

Closed
43081j opened this issue Dec 28, 2023 · 7 comments
Closed

export map means node always resolves CJS #7418

43081j opened this issue Dec 28, 2023 · 7 comments

Comments

@43081j
Copy link
Contributor

43081j commented Dec 28, 2023

Describe the bug

given:

"types": "./dist/types/index.d.ts",
"node": "./dist/cjs/index.js",
"require": "./dist/cjs/index.js",
"default": "./dist/esm/index.js"

node will always resolve to the CJS entrypoint, regardless of if you're in an ESM package or not.

i think the correct map would be:

 "types": "./dist/types/index.d.ts",
 "require": "./dist/cjs/index.js",
 "default": "./dist/esm/index.js"

but this is also a lie since rxjs is a CJS package (type is not "module" in package.json), so it should probably be:

 "types": "./dist/types/index.d.ts",
 "import": "./dist/esm/index.js",
 "default": "./dist/cjs/index.js"

at this point, we will resolve to the correct entrypoint but types will still be wrong with nodenext resolution.

fixing type resolution

when using nodenext resolution in typescript, the above map will be considered an error ("masquerading" modules). this is because CJS and ESM entrypoints shouldn't have the same type definitions.

you can read about that here.

the only solution to this really is to publish double the types unfortunately (or stop shipping CJS...), so for a fully valid export map, we would have:

 "import": {
    "types": "./dist/types-esm/index.d.mts",
    "default": "./dist/esm/index.js"
  },
 "require": {
    "types": "./dist/types/index.d.ts",
    "default": "./dist/cjs/index.js"
  }

Note the file extension, this is important since node will otherwise treat it as commonjs (because "type" is not "module", it will treat *.js as CJS, and typescript will treat *.d.ts as CJS types).

Because our sources are basically being compiled into both ESM and CJS (from the same source file), this is a bit of a pain to do as typescript doesn't let you configure the output file extension (and arguably shouldn't).

In an ideal world, all ESM would be written as .mts source files, and all CJS would be .ts source files. TypeScript would then automatically emit .mjs and .js respectively.

This doesn't work in our case ofc, since the sources are the same for both. So we'd need a build script to do it.

Notes

I'm happy to fix any or all of this stuff (and have already on a branch, but needs picking apart into smaller individual branches). Just need to know a rough direction we want to go, especially for the types stuff.

Expected behavior

N/A

Reproduction code

No response

Reproduction URL

No response

Version

8.*

Environment

No response

Additional context

No response

@43081j
Copy link
Contributor Author

43081j commented Dec 28, 2023

I have opened #7419 to fix the entrypoints at least, though i still think that is the wrong way around until we move to being esm-first ("type": "module"). so i have left it as a draft for now until i get feedback here on direction

@kwonoj
Copy link
Member

kwonoj commented Dec 28, 2023

This is expected; our esm output is currently not a spec compliant esm and to support tree shaking only for bundlers. There is ongoing effort to make these output into real esm in a major / breaking changes. Before then if node.js resolves to current esm it'll fail to load (since it's not a real esm). Please search existing issues for details / history.

@43081j
Copy link
Contributor Author

43081j commented Dec 28, 2023

Do the current issues take the nodenext problems into account? Any references would be helpful as I didn't find any pointing that out so far.

There are two problems here:

  • the map always resolves to cjs
  • nodenext resolution will fail even if you fix this

So if you can point me at the "ongoing efforts", I'll write that problem up in the same tracking issue, and will be happy to help those efforts

@kwonoj
Copy link
Member

kwonoj commented Dec 28, 2023

Ongoing effort -> the plan we turn our esm into spec complaint esm eventually. If you search esm in gh issues there are discussions / histories..

So yes, map always resolves to cjs is problem. It is coming from we are yet to support spec complaint esm. It is known.

@43081j
Copy link
Contributor Author

43081j commented Dec 28, 2023

Problem one of two, yes. If you can point me at the right issue for tracking this overall, I can explain the nodenext issues there instead. Otherwise I can open a separate issue only for the nodenext problem.

Even when you fix these mappings, it will fail in typescript because of the problems I mentioned in the original post. Specifically you will need to ship two sets of types if you insist on a dual package still.

Where would you rather that discussion/issue live? It is not the same as fixing this export map

@kwonoj
Copy link
Member

kwonoj commented Dec 28, 2023

If you're referring this

when using nodenext resolution in typescript, the above map will be considered an error ("masquerading" modules). this is because CJS and ESM entrypoints shouldn't have the same type definitions.

It is also known. We are aware of those recommendation from https://arethetypeswrong.github.io/ as well, and the way we accept for now is pretty much similar to https://blog.isquaredsoftware.com/2023/08/esm-modernization-lessons/#typescript-declarations.

This may need to be fixed one day, but it's not an immediate priority.

I'm going to close this issue for now; there's a couple of spec compliant esm support tracking issue. Please feel free to append into existing comment. For me both belongs to esm support umbrella issue.

@kwonoj kwonoj closed this as completed Dec 28, 2023
@43081j
Copy link
Contributor Author

43081j commented Dec 28, 2023

Yes exactly that, it does need fixing once the exports map and file extensions have also been fixed.

I was hoping you had one umbrella issue for this stuff but maybe not. I'll have another dig around unless you can point me at the issue for tracking esm support. No worries if you don't know off the top of your head.

I'll write up this stuff into the right one when I find it. I'm here offering to help whatever these ongoing efforts are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants