TypeScript detection filtering 'node_modules'.

[holloway]

See #5947.

verifyNoTypeScript checks whether there are any TypeScript files in the project during the build, and this PR refines that logic by adding a Globby negation filter so that any node_modules under ./src is excluded from this logic.

Considering that verifyNoTypeScript has only existed since CRA 2.1 it seems very unlikely that anyone would depend on checking in a node_modules for TypeScript support, so this is a safe change.

Steps to reproduce this bug and to show that this PR fixes it:

1. symlink ./src/anything to a directory outside the CRA project directory called anything.
2. npm init the anything with its own package.json that depends on big-integer. big-integer is distributed on NPM with TypeScript files which will be present at ./src/anything/node_modules/big-integer/BigInteger.d.ts.
3. Run yarn build and CRA ignores the TS and successfully builds.

[holloway]

Sorry @mrmckeb but I don't seem to have permissions to assign this PR to you.. I hope this is good enough :)

[Timer]

I'm good with this change (probably), but linking source code into your src/ isn't a supported workflow by Create React App.
When is linking a package into src/ a good use case?

[holloway]

@Timer thanks for considering this. It seems like a harmless change because any TS project wouldn't rely on TS files only existing in node_modules, and it would let me upgrade to 2.1 so I'd appreciate it.

To answer your question the #5947 issue has more info, and I'm ok with changing to yarn link etc if that's necessary, but I couldn't find any guidance in the CRA docs about supported workflows with regard to symlinks. In earlier issues like #3547 you mention that symlinks should be working, but that's just one issue from a year ago so I don't know how relevant that is.

The project I'm on has been using CRA since "react-scripts": "0.9.0". It has a project structure of top-level directories "global", "server", and "web" (CRA)... with "server" and "web" both importing "global" for some constants and utility functions. Symlinking "global" was a workaround for later versions of CRA disallowing importing outside src/. If this PR isn't merged then I'd appreciate some guidance on how I should change the project. Thanks @Timer !

[Timer]

Yeah, this is harmless -- I think what you're doing now should suffice until we officially release create-react-something-to-handle-this. :-)

[Timer]

Can we add a e2e test for this please?

[holloway]

@Timer Sure, is there a comparable test I could look at for reference? I haven't added tests to CRA before, and I'm not quite sure where to start.

I see that there's react-scripts/fixtures/kitchensink so I guess the tests go there, but I don't see any tests for the 3 files in utils (createJestConfig.js or verifyPackageTree.js or verifyTypeScriptSetup.js).

[Timer]

<rootDir>/test/ is probably more appropriate for this. Let me know if the existing tests there aren't clear enough.

[18394093929]

。。。

[holloway]

@Timer @mrmckeb Ready for review

[mrmckeb]

Hi @holloway, sorry I missed this - December was a busy period for everyone.

This looks good, and I'm glad @Timer was able to provide some good feedback.

I only have one question/concern - what is this file for? Is it erroneous or intentional? Thanks!
test/fixtures/issue-5947-not-typescript/src/index.js

[holloway]

@mrmckeb Ah I thought it would need a src/index.js but good point it doesn't. I've removed that and changed the test accordingly.

[mrmckeb]

Thanks @holloway! Have a great day.