Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cli): change react to create-react-app #1978

Merged

Conversation

MrFix93
Copy link
Contributor

@MrFix93 MrFix93 commented Jan 21, 2020

As reported in issue #1435, the react preset is only applicable to a react project set up by create-react-app. It causes some confusion whereas the error (unable to locate package react-scripts) isn't that useful either. Therefore, I propose we change the projectType to react-create-app as it resembles the applicability of this preset.

For backwards compatibility, "react" and "react-ts" can still be used, however, it does log a deprecation warning.

ps. First PR 馃殌

As reported (stryker-mutator#1435) the react preset is only applicable to a react project set up by create-react-app.
In addition, the error (unable to locate package react-scripts) isn't that useful either.
Therefore, I propose we change the projectType to react-create-app as it resembles the applicability of this preset.
@mthmulders
Copy link
Contributor

Lookin' good!

Now that create-react-app also supports TypeScript, do the defaults of the create-react-app case still make sense? We could be seeing projects that use create-react-app (not create-react-app-ts) and use TypeScript at the same time.

@nicojs
Copy link
Member

nicojs commented Jan 21, 2020

@mthmulders good catch!

See #1362 about CRA with TypeScript. This was still an open question from my end.

@mthmulders or @MrFix93 do you know what the correct defaults are for different CRA setups? For example, our vue-cli has some more questions is asks before writing the config. We could do the same thing for CRA.

@mthmulders
Copy link
Contributor

@mthmulders or @MrFix93 do you know what the correct defaults are for different CRA setups? For example, our vue-cli has some more questions is asks before writing the config. We could do the same thing for CRA.

I don't really know. I think the biggest difference is whether one uses TypeScript. If so, we should have Stryker use the TypeScript mutators instead of the JavaScript ones. Other than that none that I'm aware of.

@MrFix93
Copy link
Contributor Author

MrFix93 commented Jan 21, 2020

Good question @mthmulders. While I was implementing this, I questioned whether the react-ts is used at all. In addition, @nicojs, it looks like the only difference in both presets (jsx and tsx React) is that tsx uses react-scripts-ts instead of react-scripts. And like the source you mentioned in #1362 , it is all bundled in react-scripts. The difference is in Stryker, as it requires the typescript mutator instead of the javascript mutator.

So. We do need to know if the user wants tsx support, as it requires different dependencies (the typescript mutator). However, the create-react-app-ts preset in the jest-runner might be abandoned. Any views on this?

In addition, it doesn't look like jest-runner supports Typescript and Javascript in a single project. Is that correct?

@nicojs
Copy link
Member

nicojs commented Jan 21, 2020

However, the create-react-app-ts preset in the jest-runner might be abandoned. Any views on this?

That's an interesting idea. Let's do that in a future PR. It's smart to keep features in separate PR's.

The difference is in Stryker, as it requires the typescript mutator instead of the javascript mutator.

In that case I would expect there to be 1 question after you choose for CRA. Namely: what source code language do you use. @MrFix93 do you mind adding that?

Thanks again for your work so far!

@nicojs
Copy link
Member

nicojs commented Jan 21, 2020

@MrFix93 If you want, we can also merge this PR and add the option in a separate PR. It's up to you

@MrFix93
Copy link
Contributor Author

MrFix93 commented Jan 21, 2020

In that case I would expect there to be 1 question after you choose for CRA. Namely: what source code language do you use. @MrFix93 do you mind adding that?

There is already a question after the preset:
https://github.com/stryker-mutator/stryker/blob/97eed0853195089eb8d70e4ded9562200df5207d/packages/core/src/initializer/presets/ReactPreset.ts#L41

That will suffice, right?

If so, let's merge 馃殌

@nicojs
Copy link
Member

nicojs commented Jan 21, 2020

Ah yes, damn we we're smart in the past 馃槄

@nicojs nicojs merged commit 7f34f28 into stryker-mutator:master Jan 21, 2020
@nicojs
Copy link
Member

nicojs commented Jan 21, 2020

@MrFix93 congrats on your first PR 馃帀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants