Cache not invalidating declarations when import changes (need to use clean: true)

[ymoran00]

What happens and why it is wrong

I have a React project with compound componetns. Meaning - I'm using this kind of declaration:

import {Child1} from './child1';
import {Child2} from './child2';

export const Parent = ({children}:{children:ReactNode}): JSX.Element => {
  return (
    <div>
      {children}
   </div>
  );
}

Parent.Child1 = Child1
Parent.Child2 = Child2

An example of usage:

<Parent>
  <Parent.Child1 prop1="val1"/>
</Parent>

Now, if I change Child1's signature, for example changing it to expect prop2 instead of prop1 and then rebuilding, the Child1.d.ts file is updated but the Parent.d.ts file isn't. This means that I get exceptions when running the code because it still accepts the old prop1.

This means I need to run all builds with clean: true

Versions

rollup: 2.60.2
rollup-plugin-typescript2: 0.30.0
typescript: 4.5.2

rollup.config.js

`rollup.config.js`:

// noinspection JSUnusedGlobalSymbols

import commonjs from "@rollup/plugin-commonjs";
import resolve from "@rollup/plugin-node-resolve";
import external from "rollup-plugin-peer-deps-external";
import image from '@rollup/plugin-image';
import typescript from "rollup-plugin-typescript2";

// @ts-ignore
import pkg from "./package.json";

export default {
    input: "./src/index.ts",
    output: [
        {
            file: pkg.main,
            format: "cjs",
            sourcemap: true
        },
        {
            file: pkg.module,
            format: "esm",
            sourcemap: true
        }
    ],
    plugins: [
        typescript({
            clean: true
        }),
        image(),
        external({
            includeDependencies: true
        }),
        resolve(),
        commonjs()
    ]
};

tsconfig.json

`tsconfig.json` :

{
  "compilerOptions": {
    "baseUrl": "src",
    "rootDir": "src",
    "composite": true,
    "declaration": true,
    "declarationMap": true,
    "outDir": "dist",
    "declarationDir": "build",
    "module": "esnext",
    "target": "es5",
    "lib": [
      "es6",
      "dom",
      "es2016",
      "es2017"
    ],
    "sourceMap": true,
    "jsx": "react-jsx",
    "resolveJsonModule": true,
    "moduleResolution": "node",
    "allowSyntheticDefaultImports": true,
    "esModuleInterop": true,
    "typeRoots": [
      "src/@types",
      "node_modules/@types"
    ]
  }
  },
  "include": [
    "src/**/*"
  ],
  "exclude": [
    "node_modules",
    "build"
  ]
}

[agilgur5]

It's possible there's a cache bug here, potentially with how the tree is calculated (e.g. it didn't recognize that Child was changed so Parent's behavior changes), but I haven't investigated the code. Last I remember, the cache is actually fairly simple.

The other thing I noticed that could be related is that your Rollup config has node-resolve second-to-last. Per the docs, it's supposed to come before rpt2. May or may not be related.

[agilgur5]

investigated cache invalidation

As I've investigated more issues in this codebase and solved the majority of them, I kept coming back to this and was curious as to how this could happen. Especially as this isn't related to type-only files (#7).

At first I thought it was maybe because the cache invalidation seems to only take the shortest path (Dijkstra's) between dependencies instead of all paths. But that was only a quick glance; I'd think there'd be more cache issues if that were problematic.
EDIT: Reading the JSDoc for method, it gets the shortest path to all nodes from the "source", so I don't think that potential hypothesis holds up under scrutiny.

the problem is with declarations specifically

Now, if I change Child1's signature, for example changing it to expect prop2 instead of prop1 and then rebuilding, the Child1.d.ts file is updated but the Parent.d.ts file isn't. This means that I get exceptions when running the code because it still accepts the old prop1.

Reading this again, I think I've found the bug. The cache invalidation check has a flag, checkImports, which, well, does exactly what you think it does. I think you can see where I'm going from here...

Notably, this flag is set to true for type-checking (aka diagnostics), but false for compilation.

This seems to be because the JS code output shouldn't change if an import changes. That, I believe, is accurate. That's one of the reasons why Babel can just transpile TS code per-file (whereas type-checking requires knowledge of the whole "Program", i.e. the whole module graph).

But compilation also creates the declaration for the file (if one exists), and the declaration has types in it -- and so needs knowledge (and invalidation) of the module graph to be correctly output.

bug, solving has perf impact 😕

So this is a bug, and unfortunately, solving it will have a performance impact. We can limit that impact by only invalidating if declaration: true and if a file has a resulting declaration (not all do), but it'll be a de-opt either way.

not hit often?

I was surprised this hasn't been hit often, but the use-case is a bit of an edge-case. When developing, one is often not using the declaration output. That also just makes it hard to detect too -- it might actually be more frequent of an issue that just hasn't been found too much.

The declarations typically only matter during publishing, and cleaning a cache prior to publishing is always recommended. Plus, if your CI publishes it, it probably has a fresh build anyway.

But this will be particularly noticeable when working on a multi-repo or monorepo set-up, as you might be using the declaration output pretty frequently when switching between repos/individual projects. As those have become more popular, I suspect that's why this bug finally got detected.

Thanks for finding this bug and creating a simple example that illustrates it!

[agilgur5]

#369 has been released in 0.33.0