Attempt to fix custom loader race condition

[bluenote10]

This is perhaps the most naive way to fix #10871 #10315 #8707.

The idea is to make sure only one promise is created. See #10871 for details on the race condition.

I'm not an expert in webpack/fork-ts-checker-webpack-plugin and the semantics of the beforeCompile vs afterTypeScriptCheck are not entirely clear to me. In particular why beforeCompile can be called multiple times with custom loaders, but afterTypeScriptCheck is always called once. Nonetheless, the idea of the code is probably just to set a pending promise on the very first beforeCompile call, and complete it after the compilation. Since there aren't multiple calls to afterTypeScriptCheck anyway, this simple fix may be sufficient.

[facebook-github-bot]

Hi @bluenote10!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[bluenote10]

Is there anything I can do to get a review on this?

[signal-intrusion]

I'm actually not sure that this PR fixes the issue entirely. I still had race conditions in my monorepo. I was compiling a TS lib in one package and another a package running create-react-app was dependent on the compiled library code. A specific kind of type checking error would reliably cause this race condition, but not any other kind of error. Spooky.

Anyhow, I noticed that the promise sanity check would prevent the promise from being created when it should be. The fix that worked for me was to use an existing promise or create a new one like this:

    compiler.hooks.beforeCompile.tap('beforeCompile', () => {
      tsMessagesPromise = tsMessagesPromise || new Promise(resolve => {
        tsMessagesResolver = msgs => resolve(msgs);
      });
    });

[pixelkritzel]

I just applied this fix as a hotfix to my existing codebase inside the node_modules folder. It isn't stuck anymore, but also it doesn't catch typescript errors anymore either.

[signal-intrusion]

@pixelkritzel I agree with that. I tried a few more things. I've spent four full 8 hour days on this. It's just devastating. It's more than just the race condition. It seems that something deep in the compiler fails and is unable to recover.

[pixelkritzel]

@signal-intrusion

As far as I understood it, it's due to Typescripts incremental API, which is invoked by fork-ts-checker-webpack-plugin. They have an update, but it isn't part of CRA yet.

TypeStrong/fork-ts-checker-webpack-plugin#463

My personal solution is to opt out of the incremental API by patching CRA Webpack config.

https://github.com/pixelkritzel/savages/blob/master/patchWebpackConfig.js

[signal-intrusion]

@pixelkritzel I like your patch. Smaller than my fix.

I ended up taking ownership of react-scripts so that I could update fork-ts-checker-webpack-plugin. If you simply upgrade it from 4.X to 6.X and make minor changes to react-dev-utils/WebpackDevServerUtils.js and react-scripts/config/webpack.config.js the problem goes away. If the CRA team can update the fork-ts-checker-webpack-plugin dependency, they'll fix this for everyone.

For my workaround, I set create-react-app to use a local version of react-scripts, created a copy of the the parts of react-scripts and react-dev-utils that needed modification, and ones that didn't need modification are pass-through (module.exports = require('react-scripts/scripts/build.js')). So it's fixed for me, but now I own a bunch of cruft.

[danwexler]

Is there a way to use craco to workaround the issues in fork-ts-checker-webpack-plugin?

[stale]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.