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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added check for typescript template and unsupported node version #7839

Merged
merged 2 commits into from
Oct 17, 2019
Merged

added check for typescript template and unsupported node version #7839

merged 2 commits into from
Oct 17, 2019

Conversation

awaseem
Copy link
Contributor

@awaseem awaseem commented Oct 17, 2019

This PR related to #7715

Problem

When using the --typescript option with create-react-app with an unsupported version node.js ie. from the issue 8.9.4. It creates a React project, but completely ignores the TypeScript template.

Whats happening is that before this fix, if you're using an unsupported version of Node.js, createReactApp.js falls back to using version 0.9.5. Within 0.9.5, the init script (or scripts/init.js) there is no code paths that support the TypeScript option. This is why you can still get a react app project, but without TypeScript being configured.

Solution

Within createReactApp.js I just added a check and error message that tells the user they must upgrade to a supported version to continue

Testing

I tested the command using nvm, 8.10 fully created a TypeScript project but anything below exits with the following message:

You are using Node v8.9.4 with the TypeScript template. Since the old unsuporoted version of tools do not supported TypeScript, you will have to upgrade.

Please update to Node 8.10 or higher for a better, fully supported experience.

Please let me know if you have any questions!

@facebook-github-bot
Copy link

Hi awaseem! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, 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.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

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

packages/create-react-app/createReactApp.js Outdated Show resolved Hide resolved
packages/create-react-app/createReactApp.js Outdated Show resolved Hide resolved
packages/create-react-app/createReactApp.js Show resolved Hide resolved
@iansu
Copy link
Contributor

iansu commented Oct 17, 2019

We don't have any tests for this functionality. Have you tested it locally and verified things work as expected?

@awaseem
Copy link
Contributor Author

awaseem commented Oct 17, 2019

We don't have any tests for this functionality. Have you tested it locally and verified things work as expected?

I actually just tested it with my local machine. You can see it working below:

Kapture 2019-10-17 at 16 49 54

If you want I can totally write a e2e test, might just take longer

@iansu
Copy link
Contributor

iansu commented Oct 17, 2019

I'm not going to stop you from writing a test for it. 😀

But I'm okay to merge this as is. If you do want to write a test as a follow up that would be great too! Let me know what you want to do.

@awaseem
Copy link
Contributor Author

awaseem commented Oct 17, 2019

If you're comfortable with merging this, I can create another issue to track the e2e tests and create a PR based on that.

@iansu iansu added this to the 3.3 milestone Oct 17, 2019
@iansu iansu merged commit 4b024e9 into facebook:master Oct 17, 2019
@iansu
Copy link
Contributor

iansu commented Oct 17, 2019

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants