type-checking doesn't work on type-only modules

[Hotell]

What happens and why it is wrong

When module contains only types (type,interface), and those types are being imported in other module, this plugin will omit those completely from type-checking which causes shipping invalid code to consumers.

🎬 Whole repro can be found here: https://stackblitz.com/edit/github-kgn16z-grsqrq?file=libs/test/rollup-config.js

1. run yarn rollup:tsc (uses this plugin with rollup) -> this will produce no Type errors 🚨 (Wrong)
2. run yarn tsc:check(uses raw tsc) -> this will properly produce type errors ✅ (Correct, expected)

Source Code

// @filename src/index.ts
export * from './makeStyles';

// @filename src/makeStyles.ts

import { TestType } from './types';

export function makeStyles(): TestType {
  return {
    color: 'red',
  };
}

// @filename src/types.ts
interface CSS {
  backgroundColor: string;
}

export type TestType = {
  animationName?: CSS['background'];  // 🚨 this causes TS error
  color: string;
};

Versions

typescript: ~4.4.3 => 4.4.4 
[email protected]
[email protected]

rollup.config.js

`rollup.config.js`:

import * as path from 'path';
import ts from 'rollup-plugin-typescript2';
const repoRoot = path.resolve(__dirname, '..', '..');

const tsConfig = {
  check: true,
  tsconfig: 'libs/test/tsconfig.lib.json',
  useTsconfigDeclarationDir: true,
  verbosity: 4,
  tsconfigOverride: {
    compilerOptions: {
      rootDir: path.resolve(repoRoot, 'libs/test/src'),
      allowJs: false,
      declaration: true,
      declarationDir: path.resolve(repoRoot, 'out/tsc-dts'),
      paths: {
        test: ['libs/test/src/index.ts'],
      },
    },
  },
};

export default {
  input: path.resolve(__dirname, 'src/index.ts'),
  output: {
    file: path.resolve(repoRoot, 'out/bundle.js'),
    format: 'es',
  },
  plugins: [ts(tsConfig)],
};

tsconfig.json

`tsconfig.json`:

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "outDir": "../../dist/out-tsc",
    "types": ["node"]
  },
  "files": [
    "../../node_modules/@nrwl/react/typings/cssmodule.d.ts",
    "../../node_modules/@nrwl/react/typings/image.d.ts"
  ],
  "exclude": [
    "**/*.spec.ts",
    "**/*.test.ts",
    "**/*.spec.tsx",
    "**/*.test.tsx",
    "**/*.spec.js",
    "**/*.test.js",
    "**/*.spec.jsx",
    "**/*.test.jsx"
  ],
  "include": ["**/*.js", "**/*.jsx", "**/*.ts", "**/*.tsx"]
}

package.json

`package.json`:

plugin output with verbosity 4

plugin output with verbosity 3:

rpt2: built-in options overrides: {
    "noEmitHelpers": false,
    "importHelpers": true,
    "noResolve": false,
    "noEmit": false,
    "inlineSourceMap": false,
    "outDir": "/home/projects/github-kgn16z-grsqrq/node_modules/.cache/rollup-plugin-typescript2/placeholder",
    "moduleResolution": 2,
    "allowNonTsExtensions": true
}
rpt2: parsed tsconfig: {
    "options": {
        "rootDir": "/home/projects/github-kgn16z-grsqrq/libs/test/src",
        "sourceMap": true,
        "declaration": true,
        "moduleResolution": 2,
        "emitDecoratorMetadata": true,
        "experimentalDecorators": true,
        "importHelpers": true,
        "target": 2,
        "module": 99,
        "lib": [
            "lib.es2017.d.ts",
            "lib.dom.d.ts"
        ],
        "skipLibCheck": true,
        "skipDefaultLibCheck": true,
        "baseUrl": "/home/projects/github-kgn16z-grsqrq",
        "paths": {
            "test": [
                "libs/test/src/index.ts"
            ]
        },
        "pathsBasePath": "/home/projects/github-kgn16z-grsqrq/libs/test",
        "jsx": 4,
        "allowJs": false,
        "esModuleInterop": true,
        "allowSyntheticDefaultImports": true,
        "forceConsistentCasingInFileNames": true,
        "strict": true,
        "noImplicitOverride": true,
        "noPropertyAccessFromIndexSignature": true,
        "noImplicitReturns": true,
        "noFallthroughCasesInSwitch": true,
        "outDir": "/home/projects/github-kgn16z-grsqrq/node_modules/.cache/rollup-plugin-typescript2/placeholder",
        "types": [
            "node"
        ],
        "declarationDir": "/home/projects/github-kgn16z-grsqrq/out/tsc-dts",
        "configFilePath": "/home/projects/github-kgn16z-grsqrq/libs/test/tsconfig.lib.json",
        "noEmitHelpers": false,
        "noResolve": false,
        "noEmit": false,
        "inlineSourceMap": false,
        "allowNonTsExtensions": true
    },
    "fileNames": [
        "/home/projects/github-kgn16z-grsqrq/node_modules/@nrwl/react/typings/cssmodule.d.ts",
        "/home/projects/github-kgn16z-grsqrq/node_modules/@nrwl/react/typings/image.d.ts",
        "/home/projects/github-kgn16z-grsqrq/libs/test/src/index.ts",
        "/home/projects/github-kgn16z-grsqrq/libs/test/src/makeStyles.ts",
        "/home/projects/github-kgn16z-grsqrq/libs/test/src/types.ts"
    ],
    "typeAcquisition": {
        "enable": false,
        "include": [],
        "exclude": []
    },
    "raw": {
        "extends": "./tsconfig.json",
        "compilerOptions": {
            "outDir": "../../dist/out-tsc",
            "types": [
                "node"
            ],
            "rootDir": "/home/projects/github-kgn16z-grsqrq/libs/test/src",
            "allowJs": false,
            "declaration": true,
            "declarationDir": "/home/projects/github-kgn16z-grsqrq/out/tsc-dts",
            "paths": {
                "test": [
                    "libs/test/src/index.ts"
                ]
            }
        },
        "files": [
            "../../node_modules/@nrwl/react/typings/cssmodule.d.ts",
            "../../node_modules/@nrwl/react/typings/image.d.ts"
        ],
        "exclude": [
            "**/*.spec.ts",
            "**/*.test.ts",
            "**/*.spec.tsx",
            "**/*.test.tsx",
            "**/*.spec.js",
            "**/*.test.js",
            "**/*.spec.jsx",
            "**/*.test.jsx"
        ],
        "include": [
            "**/*.js",
            "**/*.jsx",
            "**/*.ts",
            "**/*.tsx"
        ],
        "compileOnSave": false
    },
    "errors": [],
    "wildcardDirectories": {
        "/home/projects/github-kgn16z-grsqrq/libs/test": 1
    },
    "compileOnSave": false
}
rpt2: typescript version: 4.4.4
rpt2: tslib version: 2.3.1
rpt2: rollup version: 2.65.0
rpt2: rollup-plugin-typescript2 version: 0.31.1
rpt2: plugin options:
{
    "check": true,
    "tsconfig": "libs/test/tsconfig.lib.json",
    "useTsconfigDeclarationDir": true,
    "verbosity": 4,
    "tsconfigOverride": {
        "compilerOptions": {
            "rootDir": "/home/projects/github-kgn16z-grsqrq/libs/test/src",
            "allowJs": false,
            "declaration": true,
            "declarationDir": "/home/projects/github-kgn16z-grsqrq/out/tsc-dts",
            "paths": {
                "test": [
                    "libs/test/src/index.ts"
                ]
            }
        }
    },
    "clean": false,
    "cacheRoot": "/home/projects/github-kgn16z-grsqrq/node_modules/.cache/rollup-plugin-typescript2",
    "include": [
        "*.ts+(|x)",
        "**/*.ts+(|x)"
    ],
    "exclude": [
        "*.d.ts",
        "**/*.d.ts"
    ],
    "abortOnError": true,
    "rollupCommonJSResolveHack": false,
    "transformers": [],
    "tsconfigDefaults": {},
    "objectHashIgnoreUnknownHack": false,
    "cwd": "/home/projects/github-kgn16z-grsqrq",
    "typescript": "version 4.4.4"
}
rpt2: rollup config:
{
    "external": [],
    "input": "/home/projects/github-kgn16z-grsqrq/libs/test/src/index.ts",
    "plugins": [
        {
            "name": "rpt2"
        },
        {
            "name": "stdin"
        }
    ],
    "output": [
        {
            "file": "/home/projects/github-kgn16z-grsqrq/out/bundle.js",
            "format": "es",
            "plugins": []
        }
    ]
}
rpt2: tsconfig path: /home/projects/github-kgn16z-grsqrq/libs/test/tsconfig.lib.json
rpt2: included:
[
    "*.ts+(|x)",
    "**/*.ts+(|x)"
]
rpt2: excluded:
[
    "*.d.ts",
    "**/*.d.ts"
]
rpt2: Ambient types:
rpt2:     /home/projects/github-kgn16z-grsqrq/node_modules/@nrwl/react/typings/cssmodule.d.ts
rpt2:     /home/projects/github-kgn16z-grsqrq/node_modules/@nrwl/react/typings/image.d.ts
rpt2:     /home/projects/github-kgn16z-grsqrq/node_modules/@types/node/index.d.ts
rpt2: transpiling '/home/projects/github-kgn16z-grsqrq/libs/test/src/index.ts'
rpt2:     cache: '/home/projects/github-kgn16z-grsqrq/node_modules/.cache/rollup-plugin-typescript2/rpt2_4b2584b8fec2cb2070a57cc5fc1737a58e40ee79/code/cache/c054c5afccc541591135fb48ef93844cf144244d'
rpt2:     cache hit
rpt2:     cache: '/home/projects/github-kgn16z-grsqrq/node_modules/.cache/rollup-plugin-typescript2/rpt2_4b2584b8fec2cb2070a57cc5fc1737a58e40ee79/syntacticDiagnostics/cache/c054c5afccc541591135fb48ef93844cf144244d'
rpt2:     cache hit
rpt2:     cache: '/home/projects/github-kgn16z-grsqrq/node_modules/.cache/rollup-plugin-typescript2/rpt2_4b2584b8fec2cb2070a57cc5fc1737a58e40ee79/semanticDiagnostics/cache/c054c5afccc541591135fb48ef93844cf144244d'
rpt2:     cache hit
rpt2: generated declarations for '/home/projects/github-kgn16z-grsqrq/libs/test/src/index.ts'
rpt2: dependency '/home/projects/github-kgn16z-grsqrq/libs/test/src/makeStyles.ts'
rpt2:     imported by '/home/projects/github-kgn16z-grsqrq/libs/test/src/index.ts'
rpt2: resolving './makeStyles' imported by '/home/projects/github-kgn16z-grsqrq/libs/test/src/index.ts'
rpt2:     to '/home/projects/github-kgn16z-grsqrq/libs/test/src/makeStyles.ts'
rpt2: transpiling '/home/projects/github-kgn16z-grsqrq/libs/test/src/makeStyles.ts'
rpt2:     cache: '/home/projects/github-kgn16z-grsqrq/node_modules/.cache/rollup-plugin-typescript2/rpt2_4b2584b8fec2cb2070a57cc5fc1737a58e40ee79/code/cache/f1f8bd7cd1ae728a9fe42101454aeb9263d56bc2'
rpt2:     cache hit
rpt2:     cache: '/home/projects/github-kgn16z-grsqrq/node_modules/.cache/rollup-plugin-typescript2/rpt2_4b2584b8fec2cb2070a57cc5fc1737a58e40ee79/syntacticDiagnostics/cache/f1f8bd7cd1ae728a9fe42101454aeb9263d56bc2'
rpt2:     cache hit
rpt2:     cache: '/home/projects/github-kgn16z-grsqrq/node_modules/.cache/rollup-plugin-typescript2/rpt2_4b2584b8fec2cb2070a57cc5fc1737a58e40ee79/semanticDiagnostics/cache/f1f8bd7cd1ae728a9fe42101454aeb9263d56bc2'
rpt2:     cache hit
rpt2: generated declarations for '/home/projects/github-kgn16z-grsqrq/libs/test/src/makeStyles.ts'
rpt2: generating target 1
rpt2: rolling caches
rpt2: generating missed declarations for '/home/projects/github-kgn16z-grsqrq/node_modules/@nrwl/react/typings/cssmodule.d.ts'
rpt2: generating missed declarations for '/home/projects/github-kgn16z-grsqrq/node_modules/@nrwl/react/typings/image.d.ts'
rpt2: generating missed declarations for '/home/projects/github-kgn16z-grsqrq/libs/test/src/types.ts'
rpt2: emitting declarations for '/home/projects/github-kgn16z-grsqrq/libs/test/src/index.ts' to '/home/projects/github-kgn16z-grsqrq/out/tsc-dts/index.d.ts'
rpt2: emitting declarations for '/home/projects/github-kgn16z-grsqrq/libs/test/src/makeStyles.ts' to '/home/projects/github-kgn16z-grsqrq/out/tsc-dts/makeStyles.d.ts'
rpt2: emitting declarations for '/home/projects/github-kgn16z-grsqrq/libs/test/src/types.ts' to '/home/projects/github-kgn16z-grsqrq/out/tsc-dts/types.d.ts'
created out/bundle.js in 949ms

Related issues

#211

[ezolenko]

Yep, looks like a bug -- rollup never processes types.ts file, which is expected because makeStyles.ts doesn't import anything that would show up in js. But typescript should have flagged the error when processing makeStyles.ts itself...

[agilgur5]

This might be the root cause underlying #212 as well 🤔 (EDIT: It's related, but not quite the root cause. See my comment there for more details)

Also thanks for providing a repro!

[agilgur5]

@ezolenko from staring at the code for a while as well as the log, it looks like TS did find it (it's in parsedConfig.fileNames), but the diagnostics for the file aren't being printed.

If I'm understanding how Rollup works correctly, since it would be tree-shaken, it doesn't show up in the resolve, transform, etc hooks, so it would rely on the dependency tree walk in the cache to find it. Looking at the log, it indeed looks like it wasn't resolved, and so wasn't set as a dependency in the cache.
I think the rest of the files need to be iterated on too for the purpose of type-checking (not just creating declarations). I suppose we could add that in _ongenerate?

#212 may have a similar (but not same) cause, in that TS finds the .d.ts files, but diagnostics aren't being printed for it for one reason or another (it's ambient or it's filtered out)

[agilgur5]

Oh apparently this a long-standing, known issue: this duplicates #7 (and apparently I've linked to that before too 😅 ).

Going to close this as a duplicate as such, though would still be curious to hear from @ezolenko about what you think about potential solutions for this long-standing issue. Something I'd like to get fixed too.

EDIT: this isn't actually a duplicate of #7. While closely related, #7 is more specific to type-only imports when it comes to cache invalidation. See below for more details

[agilgur5]

root cause analysis complete

If I'm understanding how Rollup works correctly, since it would be tree-shaken, it doesn't show up in the resolve, transform, etc hooks, so it would rely on the dependency tree walk in the cache to find it. Looking at the log, it indeed looks like it wasn't resolved, and so wasn't set as a dependency in the cache.

Been looking into this issue on and off the past month (and even well before this issue), and think I've finally wrapped my head around this.

Here's a sequence:

1. Rollup walks the import tree based on the initial file(s) it's given in input
2. The input file(s) is written in TS and so rpt2 processes the file based on its resolveId hook.
3. It then goes through the transform hook, which is where it gets a bit hairy.
   1. When the input file imports a type-only file, TS transforms this to a JS file that does not import the type-only file (see also Why are interface declarations removed? #24 (comment)). This makes sense, since a type-only file doesn't produce any JS.
   2. The transformed JS code is then passed back to Rollup (and further plugins), and importantly, does not contain the type-only import anymore. So this means resolveId is never called on the type-only file as Rollup never even sees it.

So this isn't actually due to Rollup's tree-shaking per se, type-only files are actually never part of Rollup's import tree at all (tree-shaking can impact this in other cases though, such as when there's an import in dead code).

solving the issue

solving for include

I think the rest of the files need to be iterated on too for the purpose of type-checking (not just creating declarations). I suppose we could add that in _ongenerate?

I believe this is in fact fine to do, and I have a PR about to go out for this, but this doesn't solve the totality of the issue.

In particular, it doesn't solve #211 (which is referenced here).
The declarations emitted in _onwrite are only those found in parsedConfig.fileNames. Basically everything in include's globs and files (see also #106 (comment)).
So if you're using the default include or something like src/**, the above fix should be enough for your case as your type-only files will be included in that list.
But if you're using something like files: ["src/index.ts"], it may still miss imports of type-only files as they won't be in parsedConfig.fileNames and still won't be resolved.

solving for files

This is a good bit more complicated and will need more testing, but we can find the type-only imports in rpt2 itself without relying on Rollup's resolution.

ts.preProcessFile efficiently finds all imports and references in a TS file, so we can scan the import tree using that recursively. (those docs are very sparse, so can also see the tests for ts.preProcessFile for usage details).

I then found that ts.preProcessFile is already used in rpt2, in part for a similar reason in watch mode (see also #280 (comment)).

Importantly, ts.preProcessFile can give back all sorts of imports and references, like declaration files, JS files, other filtered out files, externals, etc, so the results should really be resolved by Rollup's algorithm and all the plugins (which would come back to rpt2 for TS files, as intended).
I think this is possible using Rollup's this.resolve though. So if we just map/filter the imports and references through this.resolve, I think we'll have a correct list -- but that part I'm most skeptical will work so simply.
From there, we type-check and emit declarations for any TS files, including type-only ones. Should skip any files that have already been processed. Could potentially use Rollup's this.load which would have the files go through the transform hook

With this we can also solve #7 properly (which I now realize is more specific to watch mode and cache invalidation than all type-only issues), as not only will we have importee and importer from that, but this.resolve should actually call the resolveId hook directly, which will itself set the dependencies, so no extra work needed on that front.

So the details aren't fully hashed out yet, but this does seem possible at least

[agilgur5]

#345 should fix this specific issue as it relies on include globs and not files (i.e. the first case above)

[agilgur5]

#345 has been released in 0.33.0