fix: exclude .spec and .test files from type checking

[jwalton9]

This PR fixes react-scripts@5 including test files when type checking during npm start and npm build. This was changed in the upgrade to webpack@5 https://github.com/facebook/create-react-app/pull/11201/files#diff-8e25c4f6f592c1fcfc38f0d43d62cbd68399f44f494c1b60f0cf9ccd7344d697R732 however the syntax does not exclude .spec and .test files.

There is a reproduction repo at https://github.com/jwalton9/cra-test-type-error which introduces a type error to src/App.test.tsx. Running npm start causes these issues to be reported.

The changes were validated by using npm link and running npm start

Fixes #11979

[facebook-github-bot]

Hi @jwalton9!

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 [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta 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) Meta Open Source project. Thanks!

[gargrave]

Is there possibly an ETA on when this will be merged? This is a pretty sticky issue for us in trying to upgrade from 4 to 5.

[matthew-gladman-oua]

As a temporary work around, if you are using Craco, you can use this configuration file to inject the exclude without the typo in it:

// Taken from the original webpack config
const ForkTsCheckerWebpackPlugin =
    process.env.TSC_COMPILE_ON_ERROR === 'true'
        ? require('react-dev-utils/ForkTsCheckerWarningWebpackPlugin')
        : require('react-dev-utils/ForkTsCheckerWebpackPlugin');

module.exports = {
    webpack: {
        configure: (webpackConfig, {env, paths}) => {
            webpackConfig.plugins.map((plugin) => {
                if (plugin instanceof ForkTsCheckerWebpackPlugin) {
                    plugin.options.issue.exclude.push({file: '**/src/**/?(*.){spec,test}.*'});
                }
                return plugin;
            })

            return webpackConfig;
        },
    },
};

[jwalton9]

@iansu @mrmckeb Any chance this could be reviewed soon or at least an update on when we can expect this to be reviewed? Seems like a pretty simple fix, and I'd like to remove our custom config to get around this issue

[lejahmie]

Seems like an easy review, any actions? Please :-)

[questicles]

Please can this be merged. I've never seen a 1 character change PR that is so impactful sit idle for so long.

[WTKersten]

Yes please, it is blocking us from upgrading.

[pvsleeper]

PLEASE can someone merge this? This is making our lives hell! :(

[TheMagnificent11]

Please merge as this is also blocking us from upgrading

[XiLiuRoy]

Please merge this PR as it is blocking us as well, many thanks

[colin-byrne-1]

please merge!

[tonyjmnz]

@iansu @mrmckeb this change seems to be impactful for a number of users (myself included) and it has been sitting idle for close to a year. IMO this should be merged or closed with directions for a workaround.

[nwmahoney]

@iansu @mrmckeb +1 to what @tonyjmnz said. I'm surprised by the lack of communication on this issue. I may be misunderstanding, but this one-character change seems to be fixing what amounts to a typo.

[JeanOncle]

Please merge this issue, as this is blocking to us as well...

[florinkrasniqi1]

@jwalton9 how come this isn't reviewed at all? Did you manage to find anything else regarding this issue? I do understand that comma is interpreted as a literal character, rather than as a separator between two options and that's why they used | instead, however changing it manually seems to fix the issue for me too

[Mboulianne]

Was there any announcement, post, github issue where they explain what's going on with CRA? No release since April 2022 and PRs aren't getting approved. I see people wanting to contribute to the project, but they get no support from the code owners.

Here we have a very simple PR that would solve an annoying issue and it def shouldn't take one year to get it MERGED. Not approved, MERGED,

[StefanSchoof]

@Mboulianne I think you read #11768

[jwalton9]

I don't think this is ever going to be reviewed, so I'm going to close it. FWIW we've migrated to vite for our client rendered applications