ReactiveX/rxjs

export map means node always resolves CJS

43081j opened this issue · 7 comments

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

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

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.

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

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.

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

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.

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.