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

Can't correctly import plugins into a TypeScript module #1541

Open
lazarljubenovic opened this issue Jul 20, 2023 · 6 comments · May be fixed by #1782
Open

Can't correctly import plugins into a TypeScript module #1541

lazarljubenovic opened this issue Jul 20, 2023 · 6 comments · May be fixed by #1782

Comments

@lazarljubenovic
Copy link

lazarljubenovic commented Jul 20, 2023

  • Rollup Plugin Name: at least typescript and node-resolve, probably all
  • Rollup Plugin Version: typescript at 11.1.2, node-resolve at 15.1.0 (currently latest)
  • Rollup Version: 3.25.1 (currently latest)
  • Operating System (or Browser): Ubuntu
  • Node Version: 16.14.1
  • Link to reproduction (⚠️ read below): https://github.com/lazarljubenovic/issue-repro-rollup-plugin

The repo is really minimal. The important part is that it's a module:

https://github.com/lazarljubenovic/issue-repro-rollup-plugin/blob/097e003ae506342974740a58788e65f7a951ad40/package.json#L5

And that I'm just trying to import and invoke it.

https://github.com/lazarljubenovic/issue-repro-rollup-plugin/blob/097e003ae506342974740a58788e65f7a951ad40/index.ts#L1-2

Expected Behavior

It compiles.

Actual Behavior

It gives the following type 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.

The generated codes works, though -- the function call is successfully performed and returns the correct value.

Additional Information

The issue is happening because the type of my import is treated as if it was written in CommonJS. In other words, even thought the runtime will perform import typescript from '@rollup/plugin-typescript', types are resolved as if I've written import typescript = require('@rollup/plugin-typescript'). This explains the error, where TS believes that typescript is the whole module. In fact, if I change my code to typescript.default(), it compiles but fails at runtime because the default export is a function without a property default.

The root cause is simply the way TS works at the moment. It's a surprising behvaior, but it's documented:

It’s important to note that the CommonJS entrypoint and the ES module entrypoint each needs its own declaration file, even if the contents are the same between them. Every declaration file is interpreted either as a CommonJS module or as an ES module, based on its file extension and the "type" field of the package.json, and this detected module kind must match the module kind that Node will detect for the corresponding JavaScript file for type checking to be correct. Attempting to use a single .d.ts file to type both an ES module entrypoint and a CommonJS entrypoint will cause TypeScript to think only one of those entrypoints exists, causing compiler errors for users of the package.

Since the extension is .d.ts, and the rollup plugin package is not a module, TypeScript determines that .d.ts is always a CJS module. The exports.types setting is correctly used, but what it points to is interpreted as CJS.

Therefore, the package must be distributed with two distinct declaration files. For example, after making the following changes in my node_modules folder, everything works correctly.

  + types
- +-- index.d.ts
+ +-- index.d.mts
+ +-- index.d.cts
  "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"
  },

The contents of index.d.mts and index.d.cts are identical to the original index.d.ts.

This works with both "type": "module" and "type": "commonjs".

// with "type": "module", this works now
import typescript from '@rollup/plugin-typescript'
typescript()
// with "type": "commonjs", this also works
import typescript = require('@rollup/plugin-typescript')
typescript.default()
@shellscape
Copy link
Collaborator

We've never really had an issue with imports of plugins in TS. If you feel there's something actionable here, please feel free to open a PR that's backwards compatible with tests around your changes.

@lazarljubenovic
Copy link
Author

lazarljubenovic commented Jul 20, 2023

Managed to reproduce in StackBlitz: https://stackblitz.com/edit/rollup-repro-cf618b?file=src%2Fmain.ts

Run npx tsc in the terminal and you'll see the compilation error. Not sure why the StackBlitz editor itself doesn't catch the error. My WebStorm shows it properly when I reproduce locally.

@lazarljubenovic
Copy link
Author

I'll try spinning up a PR later, I just need to find time to figure out the build process. It's a trivial fix apart from getting to know the project (I see some "pnpm" shenanigans from the package.json).

@simonbuchan
Copy link
Contributor

Are The Types Wrong will tell you when and why the types are wrong, if it helps.

@matthieusieben
Copy link
Contributor

Here is a small workaround in mjs:

import { defineConfig } from 'rollup'

import commonjs from '@rollup/plugin-commonjs'
import json from '@rollup/plugin-json'
import nodeResolve from '@rollup/plugin-node-resolve'
import resolve from '@rollup/plugin-replace'

const NODE_ENV =
  process.env['NODE_ENV'] === 'development' ? 'development' : 'production'

/**
 * @template T
 * @param {{ default: T }} f
 * @see {@link https://github.com/rollup/plugins/issues/1541}
 */
const fix = (f) => /** @type {T} */ (f)

export default [
  defineConfig({
    input: 'dist/app.js',
    output: { format: 'iife', file: 'dist/app.bundle.js' },
    plugins: [
      fix(json)(),
      fix(commonjs)(),
      fix(nodeResolve)({ preferBuiltins: false, browser: true }),
      fix(resolve)({
        preventAssignment: true,
        values: { 'process.env.NODE_ENV': NODE_ENV },
      }),
    ],
  }),
]

@43081j
Copy link
Contributor

43081j commented Dec 11, 2023

if i understood correctly, this PR will produce:

  • dist/cjs/index.js
  • dist/es/index.js
  • dist/cjs/index.d.ts (copy of types/index.d.ts)
  • dist/es/index.d.ts (copy of types/index.d.ts)

is that actually right? don't we actually need:

  • dist/cjs/index.js
  • dist/es/index.js
  • dist/cjs/index.d.ts
  • dist/es/index.d.mts

since we're in a CJS package, i would've assumed typescript would default to *.d.ts being commonjs here

so ATTW would still complain about cjs masquerading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment