Improved detection logic for projects created by create-react-app

[stephtr]

The old logic for detecting react-scripts or similar had a flaw (#244): When testing for the presence of for example react-scripts-ts, on Windows it checked the existence of a folder named node_modules/react-scripts-ts.cmd. Furthermore that approach is not compatible with Yarn workspaces, which usually places the folder in a parent directory.

Instead it is better to check for the existence of the binary within node_modules/.bin. Another, in my opinion more stable, approach would be to parse package.json and check its dependencies.

[orta]

I agree, this makes sense 👍

[rrousselGit]

How will this be compatible with other custom CRA ? react-scripts-ts isn't the only one existing

[rrousselGit]

I fear that the following

const packageNames = ['react-scripts', 'react-native-scripts', 'react-scripts-ts']

is not a good idea considering what I said above

After all, the recommended way to add custom configs (such as react-hot-loader or sass, or maybe other languages such as dart/reason) is to fork react-scripts.
Such hard-coded list would prevent this kind of customisation

[stephtr]

I know that, but I thought it is still less lousy and easier to extend than the previous state with four fixed paths to check for three packages.
The question is how can it be reliably detected if a project has been generated by create-react-app? Checking for the specific packages was the only solution which came to my mind. One idea was to check for the presence of scripts.test, but that doesn't tell you if that command would run something with jest.

However I just noticed that checking for the presence of the binaries could actually be more useful than parsing package's dependencies since some forks of react-scripts don't change the name of their binary. Therefore it would be probably better to remove the whole try block in favor of the last line (return packageNames.some(pkg => hasNodeExecutable(rootPath, pkg))). One problem are corner cases, in which ./node_modules/.bin/… exist, even though the project wasn't created by create-react-app (for example when using Yarn workspaces), but I guess we could tolerate some false positives for detecting more react-scripts forks without explicitly hard-coding them?

[rrousselGit]

Maybe we should considerer a setting such as jest.pathToReactScripts which would default to

[
   "node_modules/react-scripts",
   "node_modules/react-native-scripts",
   "node_modules/react-scripts-ts",
]

[orta]

I'm not adverse to that

[stephtr]

But do we gain much if the user has to specify which versions of react-scripts he may want to use compared to just setting "jest.pathToJest": "npm test --" per project?

[rrousselGit]

So far, "jest.pathToJest": "npm test --" doesn't works on my end. I get a Process failed: spawn npm ENOENT. (And to be honnest, a npm command on "pathToJest" feels weird).

And I may make a mistake but : To begin with, if "jest.pathToJest": "npm test --" is considered as a valid solution to custom tests ; what's the point of detecting if this is a create-react-app project ?

Why not just always use npm test -- on all projects ?

[stephtr]

ENOENT should be fixed with the next release (#248)

Because there exist a lot of other testing frameworks. By defaulting to npm test -- VS Code would automatically run those tests in background (and consume CPU) without this extension being able to interpret the results.
Another idea (to limit false positives) would be to also check scripts.test for known binaries, I will try to refine the logic today.

[stephtr]

#268 should fix the issue for most forks of react-scripts. Do you know any major custom CRA not covered by it?

[stephtr]

It turns out my comment about defaulting to npm test -- was incorrect, as the extension gets only triggered when the project somehow includes jest. The documentation even states This project has the expectation that you would run something like npm run test which just looks like jest

[orta]

You can also manually activate the extension with > Start Jest

[stephtr]

My thought was not about when to activate the extension but when it should call npm test and when it should call jest directly. According to the documentation it should use npm test by default as soon as there is a test script available, but currently it calls npm test only if it detects that the project has been generated by create-react-app. Therefore we should update either the documentation or (just another time) the logic behind pathToJest.

[seanpoulter]

what's the point of detecting if this is a create-react-app project?

Folks want it to be simple and work out of the box. I've added the other create-react-app derivates to the check when folks opened issues. It's low overhead to check if a file exists.

Why not just always use npm test -- on all projects?

It's a decent idea to run "npm test" if it's defined inpackage.json, and jest otherwise. We'd still need a setting if folks need extra args for the extension (e.g.: @stephtr has talked about adding args to collect coverage).

According to the documentation it should use npm test by default as soon as there is a test script available

I read it like "we expect your 'test' script in package.json to look like: "test": "jest", but I can see what you mean @stephtr. Shall we update the docs?

README.md

This project has the expectation that you would run something like npm run test which just looks like jest in the package.json. So, please keep your configuration inside the package.json as opposed to using command line arguments.

[stephtr]

We'd still need a setting if folks need extra args for the extension

My thought was to leave the pathToJest setting intact but switching the default value to npm test -- if there is a test script present or jest otherwise. That way we wouldn't have to separately address create-react-app.

Shall we update the docs?

I would either implement my thought (and refine the docs) or keep the current behavior and update the docs.

[seanpoulter]

I'm OK with changing the default, but I'd prefer if we make it a smooth migration and notify the user of the change using a message and a quick pick menu as required.

[stephtr]

In my opinion two different behaviors when using the same configuration would be suboptimal, just like adding a new setting, but let's see which direction #271 is taking. In the meantime I will create a PR with a suggestion for updating the docs.