Incorrect webpack --watch behavior for type-only imports when using yarn workspaces

[jtbandes]

Current behavior

Type-only imports (import type X, or import X where X is only used as a type) result in incorrect webpack --watch behavior. When files residing in workspace packages are modified, the importing code does not always get recompiled.

This is a similar issue to #36 and TypeStrong/ts-loader#1138, but that issue has been fixed and the recommendation to use "importsNotUsedAsValues": "preserve" has been removed in #516. However, issues still arise when using yarn workspaces.

It seems like enabling "importsNotUsedAsValues": "preserve" and avoiding import type might be a viable workaround for this issue, but that's not great, because import type is a core TS feature that should ideally be supported properly.

Expected behavior

Modifying a file in a workspace package should cause webpack to recompile the bundle, even if it's only used via type-only imports.

Steps to reproduce the issue

0. Check out https://github.com/jtbandes/ts-workspaces-repro. This repo uses Yarn workspaces and has two sub-packages under packages/pkg1 and packages/pkg2. The main file src/index.ts imports from these packages.

1. Run yarn install && yarn webpack --watch --progress --mode=development

2. Modify type Example1 in packages/pkg1/index.ts. Observe that webpack does not recompile src/index.ts nor display new type errors. ❌

3. Modify function Example2 in packages/pkg2/index.ts. Observe that webpack does recompile src/index.ts and display new type errors (this is not a type-only import). ✅

4. Add
   "compilerOptions": { "importsNotUsedAsValues": "preserve" } to tsconfig.json. Now, observe that changing pkg1/index.ts does cause webpack to recompile. ✅

   • Change import {Example1} from 'pkg1' to import type {Example1} from 'pkg1' in src/index.ts. Observe that changing pkg1/index.ts again does not result in a recompile. ❌ (This is because "importsNotUsedAsValues": "preserve" does not affect explicit import type imports, which are always removed.)

Issue reproduction repository

https://github.com/jtbandes/ts-workspaces-repro

Environment

• fork-ts-checker-webpack-plugin: 6.2.6
• typescript: 4.2.4
• ts-loader: 9.1.2
• webpack: 5.37.0
• os: macOS 11.3.1

[jtbandes]

Actually, in a larger repo I still don't get some type errors when changing files, even when I've changed all import type to import and enabled "importsNotUsedAsValues": "preserve". I see webpack rebuilding at the right time, but fork-ts-checker doesn't give me new TS errors. This is in a more complex workspace setup though — I'm not sure where the difference lies between my example repo and the problem we're seeing in production. See https://github.com/foxglove/studio/issues/893 for details

[piotr-oles]

Thanks for reporting that, I will take a look hopefully in this week :)

[jtbandes]

Thanks for the response! Please let me know if you need any more information or help debugging 🙂 I noticed that my example repo demonstrates a partial issue, but the workaround doesn't work in our actual real-world repo, so the issue might be more complex than I initially guessed.

I created a branch where I tried to remove all import type and enable "importsNotUsedAsValues": "preserve". But you can still see issues on this branch, by running yarn serve and editing app/types/panels.ts.

[piotr-oles]

Thanks again for the reproduction repository. I wish everyone would report issues as you did 😄
To make webpack compilation fast, we don't listen to node_modules directory changes. As you know, your projects are symlinked there by yarn workspaces.
The solution is to use project references. Thanks to your example I found one issue that will be solved in #615. When it will be merged, this is how you can set up your project: jtbandes/ts-workspaces-repro#1

[jtbandes]

Thanks! Is there a new build of fork-ts-checker that I can install to test out #615 in my repo?

[piotr-oles]

I just merged #615, should be released in 30 minutes

[jtbandes]

I tried 6.2.9, and improved some of the settings you recommended: https://github.com/foxglove/studio/compare/90cdcd3

However, I am now getting some errors when building. Strangely, the Referenced project '...' may not disable emit errors seem to change if I run yarn tsc -b packages/just-fetch before building, or if I find packages app -name \*.tsbuildinfo -delete. So they are related to some cached data — is there a chance fork-ts-checker or ts-loader is causing incorrect tsbuildinfo files to be saved? Note that I don't have noEmit set in any of these packages. However, I do override noEmit: true in the ts-loader options.

Errors from yarn serve on my branch above

ERROR in ../../app/AppSetting.ts
Module build failed (from ../../node_modules/ts-loader/index.js):
Error: TypeScript emitted no output for /Users/work/Desktop/studio/app/AppSetting.ts. The most common cause for this is having errors when building referenced projects.
    at makeSourceMapAndFinish (/Users/work/Desktop/studio/node_modules/ts-loader/dist/index.js:53:18)
    at successLoader (/Users/work/Desktop/studio/node_modules/ts-loader/dist/index.js:40:5)
    at Object.loader (/Users/work/Desktop/studio/node_modules/ts-loader/dist/index.js:23:5)
 @ ./telemetry.ts 7:0-61 21:41-75 22:36-64
 @ ./index.ts 19:0-51 32:30-50

ERROR in ../../app/version.ts
Module build failed (from ../../node_modules/ts-loader/index.js):
Error: TypeScript emitted no output for /Users/work/Desktop/studio/app/version.ts. The most common cause for this is having errors when building referenced projects.
    at makeSourceMapAndFinish (/Users/work/Desktop/studio/node_modules/ts-loader/dist/index.js:53:18)
    at successLoader (/Users/work/Desktop/studio/node_modules/ts-loader/dist/index.js:40:5)
    at Object.loader (/Users/work/Desktop/studio/node_modules/ts-loader/dist/index.js:23:5)
 @ ./index.ts 13:0-83 22:12-20 22:24-35 38:50-61 173:25-33 174:28-39 177:17-29

ERROR in ./index.ts 8:17-40
[tsl] ERROR in /Users/work/Desktop/studio/packages/ros1/tsconfig.json(8,18)
      TS6310: Referenced project '/Users/work/Desktop/studio/packages/xmlrpc' may not disable emit.

ERROR in ./index.ts 8:17-44
[tsl] ERROR in /Users/work/Desktop/studio/packages/xmlrpc/tsconfig.json(8,18)
      TS6310: Referenced project '/Users/work/Desktop/studio/packages/just-fetch' may not disable emit.

ERROR in ../../packages/log/src/index.ts
Module build failed (from ../../node_modules/ts-loader/index.js):
Error: TypeScript emitted no output for /Users/work/Desktop/studio/packages/log/src/index.ts. The most common cause for this is having errors when building referenced projects.
    at makeSourceMapAndFinish (/Users/work/Desktop/studio/node_modules/ts-loader/dist/index.js:53:18)
    at successLoader (/Users/work/Desktop/studio/node_modules/ts-loader/dist/index.js:40:5)
    at Object.loader (/Users/work/Desktop/studio/node_modules/ts-loader/dist/index.js:23:5)
 @ ./index.ts 14:0-35 21:12-28

webpack 5.36.0 compiled with 5 errors in 23889 ms

Edit: I tried removing noEmit: true from my ts-loader settings. This means ts files will actually be emitted in my local filesystem whenever running webpack serve - not really what I want, but that's ok. Now the build succeeds! 🎉 But it's also incredibly slow, even when I'm just doing an incremental build by changing one file. I'm also getting two copies of errors, one from ts-loader and one from fork-ts-checker, even though I set transpileOnly: true in my ts-loader options.

[jtbandes]

Here's an update to my example repo: jtbandes/ts-workspaces-repro@a4dde59

I enabled projectReferences: true for ts-loader, and upgraded to fork-ts-checker 6.2.9. Now I still do not see new type errors when I modify pkg1/index.ts.

[jtbandes]

I'm still having issues in my main repo that I unfortunately can't reproduce in the small test repo. If you have some time to try it out, we would really appreciate the help:

1. check out this repo @ cdd2deb
2. run yarn install and yarn serve
3. Edit app/types/panels.ts. Webpack rebuilds, but does not show any new type errors.

Note that this doesn't use build mode / project references. We don't think it's necessary because we don't run tsc directly for the main packages under desktop and app — we only use webpack for those. TypeScript and fork-ts-checker should be able to resolve imports correctly based on our webpack resolve.alias settings.

There is another branch (mentioned in a previous comment) where I tried using project references / build mode — but this is extremely slow and cpu intensive, and produces all the .js/.d.ts files on the filesystem, which we don't need for our webpack build.

[jtbandes]

^ This PR contains a workaround for the issue, so we now get type errors when changing type-only files and files in other workspace packages. However, it also seems to slow down the type checking step (is fork-ts-checker re-checking every file mentioned in include?) and ideally this workaround would not be necessary.

[piotr-oles]

It seems that there is another "bug" in the plug-in. When you run project references with ts-loader, ts-loader writes files faster than plugin and typescript uses mtime of file to decided if re-check is needed. I have some ideas how to fix it.

But I'm also thinking that maybe I could implement some exception for directories that are symlinked so project references would not be required here. I'm thinking about this, because I don't know what will be a performance impact of using project references :)

[johnnyreilly]

cc @sheetalkamat and @andrewbranch in case they're interested

[jtbandes]

Does this mean ts-loader and fork-ts-checker are both competing to write .js and .d.ts files if project references are used? I noticed that I did get both [tsl] and regular TS9999 error messages sometimes when using project references, which seems wasteful.

[piotr-oles]

In the plugin, I use memfs to write files, so they are not written to the real file system. The problem is that when I read them, I use "passive" file system, which uses the newest one by comparing mtime. The #617 tries to fix that by using memfs always for output files :)

[piotr-oles]

Could you try 6.2.10? It should work, but I'm also interested if it's performant in your project :)

[jtbandes]

I tried it briefly and updated this branch: https://github.com/foxglove/studio/compare/jacob/webpack-ts-fixes I started running into some new build errors but didn't have time to look into it yet (possibly I just missed one of the project dependencies).

But I think overall, using project references is less performant for us, so we would prefer if there's a way to fix the incorrect watch behavior when project references are not used...

[piotr-oles]

Ok, I think the easiest fix for you would be to use https://github.com/pigcan/extra-watch-webpack-plugin:

  plugins: [
      new ForkTsCheckerWebpackPlugin(),
      new ExtraWatchWebpackPlugin({
        files: ['packages/**/*.ts']
      })
  ],

I don't want to make this plugin too smart to detect workspaces because there are so many edge-cases. I think it makes more sense to let developers specify where they are using the extra-watch-webpack-plugin plugin. Let me know if it works in your project :)

[piotr-oles]

Or maybe you could even use built-in plugin functionality:

  plugins: [
      new ForkTsCheckerWebpackPlugin({
        typescript: {
          configOverwrite: {
            include: ["./src/**/*.ts", "./packages/**/*.ts"]
          }
        }
      })
  ]

[piotr-oles]

Or maybe you can just update tsconfig.json instead of using configOverwrite :D

[jtbandes]

Or maybe you can just update tsconfig.json instead of using configOverwrite :D

That's what I tried here: https://github.com/foxglove/studio/pull/967/files

The problem is that these changes are only required because of our webpack/fork-ts-checker setup, and they are a bit fragile/hard to maintain... and it seems like it also adds extra rebuild time due to typechecking more files. From a TypeScript standpoint these files don't need to be listed in include because the regular import resolution will take care of it based on yarn workspace links.

This is what I'm confused by: how does fork-ts-checker decide what files to watch? Is it purely based on the include settings from the tsconfig? How is it possible that sometimes webpack will rebuild when I change a file, but fork-ts-checker doesn't give me new type errors? (Are webpack and fork-ts-checker both watching their own set of files?)

[piotr-oles]

Is it purely based on the include settings from the tsconfig?

Yes. The list of files to watch cannot be provided asynchronously to webpack, so this operation is blocking. Because of that, we can't hook into TypeScript imports to get a list of files to watch. If we would do that, the async: true mode would be useless.
We have to watch because in the transpileOnly: true mode in ts-loader, typescript strips away all type-only imports. Because of that, these imports are not included in the webpack's dependency tree and therefore are not being watched.

How is it possible that sometimes webpack will rebuild when I change a file, but fork-ts-checker doesn't give me new type errors?

The plugin doesn't have its own watch system - it uses webpack's one. So on each rebuild, we get a list of changed and removed files and we send this to the plugin. Then, this list is propagated to the TypeScript instance. The situation you describe is when some files (type-only in this case) are not being watched. In this case, TypeScript will not be notified about this file change so it will keep the out-of-date version of the file in memory.

[piotr-oles]

I think adding ExtraWatchWebpackPlugin should not increase type-checking time. If it's true, then maybe we could define some reasonable default behavior with an opt-out option 🤔
Something like workspaces: true that by default add workspaces typescript files to the watch set.

Of course, the most correct way would be to use webpack API to build a proper dependency tree, but this is something that we intentionally sacrifice for performance :)

[fxmanu]

@piotr-oles any update on this?

[piotr-oles]

Could you test with the latest version? I fixed some bugs, maybe this will help

[piotr-oles]

I tested the newest version with the reproduction repository and I'm no longer able to reproduce the issue. I think #712 may fix the bug :) I'm closing this issue, let me know if this isn't the case and it still doesn't work.

[jtbandes]

Hi, thanks for the update! I tested 7.2.1 in https://github.com/foxglove/studio and I still experience the same issue: when I run yarn serve and make some changes in packages/studio-base/src/types/panels.ts, fork-ts-checker does not re-run and pick up the changes.

[didii]

Chiming in here to give another minimal repo where the issue happens. It's not necessarily related to yarn and/or workspaces. The only thing 'special' about this repo is that it is using --isolated-modules (which is required if you use babel) and thus exports its types as export type { ... } from '...';.

Whenever you change something in the options-for-a.ts interface file, no check will happen. The same happens if your comment the export type-line in the index.ts file.

Using export * from '...'; fixes the issue as does removing the --isolated-modules option together with using export { ... } from '...';. Both are not really ideal solutions :)

The repo: https://github.com/didii/fork-ts-checker-watch

[piotr-oles]

@didii In the example you posted, you could fix it by changing "include": ["src/index.ts"] to "include": ["src"] in tsconfig.json file. We compute dependencies using tsconfig.json only, because it's a blocking operation (it will block webpack emit), so we can't rely on the typescript dependency tree resolution.

[didii]

@piotr-oles Thanks, that indeed fixes the watch issue. It's not exactly how I'd want it since it will now check every file, even if it is not included (e.g. storybook files or some plain POC files), but it does suffice for dev purposes :)

Maybe this is something that should be added as a remark in the readme since it deviates from the default TSC behavior?