fix(repo): plugin type definitions not being exported correctly in ES…

[Geylnu]

Rollup Plugin type definitions

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

fixes #1541, fixes #1329

If you set moduleResolution to Node16 or NodeNext in tsconfig and import the Rollup plugin, you will get the following error:

index.ts:2:1 - error TS2349: This expression is not callable.
  Type 'typeof import("/home/lazar/dummy-01/node_modules/@rollup/plugin-typescript/types/index")' has no call signatures.

For a detailed analysis of the cause see: #1541

[tada5hi]

types for cjs are missing in the exports definition. It should look like this in the package.json files.

  "exports": {
-   "types": "./types/index.d.ts",
-   "import": "./dist/es/index.js",
+   "import": {
+     "types": "./types/index.d.mts",
+     "default": "./dist/es/index.js"
+   },
+   "require": {
+     "types": "./types/index.d.cts",
+     "default": "./dist/cjs/index.js"
+   },
    "default": "./dist/cjs/index.js"
  },

[Andarist]

types for cjs are missing in the exports definition. It should look like this in the package.json files.

Technically it doesn't but I agree that the proposed structure is just less confusing. The CJS types could be sourced from the "sibling" .d.ts file but the definition files here are not laid out on disk this way.

Overall, this PR introduces references to many non-existing files and that should definitely be fixed

[Geylnu]

types for cjs are missing in the exports definition. It should look like this in the package.json files.

  "exports": {
-   "types": "./types/index.d.ts",
-   "import": "./dist/es/index.js",
+   "import": {
+     "types": "./types/index.d.mts",
+     "default": "./dist/es/index.js"
+   },
+   "require": {
+     "types": "./types/index.d.cts",
+     "default": "./dist/cjs/index.js"
+   },
    "default": "./dist/cjs/index.js"
  },

I tested the commonjs module locally and it looks like typescript will use the types declared file as a fallback if the cjs type declaration file is not declared in the exports field, so I didn't declare the additional cjs type declaration file entry.
But @Andarist provides a better way to make declaration files self-contained without providing additional configuration

[shellscape]

@lukastaegert since this affects the entire repo and assets we publish for the entire repo, I'd like your stamp on this before w emerge. @Andarist @tada5hi you two as well please.

@Geylnu please update your branch from upstream so we can run the latest CI

[lukastaegert]

Currently, it looks like this PR is just removing types from all package files. Am I missing something here?

[Andarist]

@lukastaegert TypeScript is following the standard node resolution algorithm - with small extensions. package.json#types field isn't the only way in which it can load types for a given package. It takes priority over other things as it's a piece of explicit information~ but by default TS tries to resolve package.json#main (and probably things like pkg/index.js) and if it resolves successfully then it tries to load types using, what I call, a sibling lookup. It always tries to load foo.d.ts when it resolves to foo.js

package.json#exports.types is somewhat different. It doesn't take priority over anything else. Conditions within the exports field are resolved in the source order. It just happens that TS is always adding types condition alongside require, import, default (I hope I didn't miss any :P). That means that this is, in fact, invalid:

{
  "exports": {
    "import": "./foo.js",
    "types": "./types.d.ts"
  }
}

But the "sibling lookup" still works when reading from package.json#exports. So this PR simply uses that mechanism as it puts the declaration files as siblings to their runtime counterparts. This allows for CJS and ESM files to resolve separately without messing too much with the package.json#exports map. Without that you'd need to create smth like this:

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

Note that when publishing a dual package using a single types condition is incorrect (and this PR fixes that exact problem). If you ship ur runtime code twice then it only makes sense to do the same with types. A dual package at runtime should always be a dual package in the types realms too.

[lukastaegert]

Ah makes sense, I was missing that but of course generated files would not turn up in the diff. So it would emit the same file twice. If this fixes the issue at hand and we do not need slightly different types for CommonJS, I'm fine with this change.

[tada5hi]

For me it still feels weird not to use explicit type references in the package.json and you don't know if the sibling lookup won't get depcreated unexpectedly at some point.
I therefore have mixed feelings about this.
But I otherwise agree with @lukastaegert that if it solves the problem and everyone else is fine with it, it should not fail on me.

[Andarist]

For me it still feels weird not to use explicit type references in the package.json

Overall, I probably prefer the explicit condition as well. Although a cleaner package.json content is a nice thing about relying on this sibling lookup.

you don't know if the sibling lookup won't get depcreated unexpectedly at some point.

This is highly unlikely. It has been there almost forever and is actually a somewhat preferred way of structuring things by Andrew Branch (who is the TS team member who works a lot on the module resolution stuff).

[tada5hi]

For me it still feels weird not to use explicit type references in the package.json

Overall, I probably prefer the explicit condition as well. Although a cleaner package.json content is a nice thing about relying on this sibling lookup.

vuejs, for example, also does this with their ecosystem and imports work like a charm there.
https://github.com/vuejs/core/blob/main/packages/vue/package.json

you don't know if the sibling lookup won't get depcreated unexpectedly at some point.

This is highly unlikely. It has been there almost forever and is actually a somewhat preferred way of structuring things by Andrew Branch (who is the TS team member who works a lot on the module resolution stuff).

I would tend to doubt it too, but I wanted to note it anyway.

[Geylnu]

@shellscape I have updated my branch, is there anything else I need to do if I need this PR to be merged?

[benmccann]

The build was failing with a message that looked like it was because it was out of sync with master, so I updated it to the latest master. It's now failing with a new error, which looks like it's coming from the changes in this PR

If you're able to get the build passing, then I'll leave another approval on this PR as I'd love to see it merged

[shellscape]

Closing as abandoned.