-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Add template support #7716
Add template support #7716
Conversation
75c0ac7
to
eda43e2
Compare
@ianschmitz @iansu This PR is working locally, but I have a handful of tests to fix. It is ready for you to take a look at though, and you can test locally:
|
fbdd34c
to
3964653
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I left a few specific comments and also have a few more general thoughts:
- This should be behind a flag in 3.x
- Naming conventions
create-react-app-template-*
is super long. I don't like usingcra
but maybecra-template-*
is better?- Having to include
typescript
in the name of a TypeScript template seems not feasible. Maybe check fortypescript
in the template dependencies?
I'm going to attempt to use your branch to create a custom template next and will probably have more feedback then.
"url": "https://github.com/facebook/create-react-app/issues" | ||
}, | ||
"files": [ | ||
"template", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit weird to have a directory named template
inside the template package. Can we put files at the root or maybe in src
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried agree, and I did try that, but this made more sense to me as for whitelisting files to publish. It also means that the project can have a separate README for the published package, vs what's installed with the template.
3964653
to
3809678
Compare
d1f43b5
to
6998bc6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm - --scripts-version
will still work by itself until v4, correct?
@ianschmitz Correct, everything works without a template for now. A custom script would need to accept the template though, otherwise it would just use the template it has. |
First of all, awesome work! this will be a sweet addition to let others Kickstart apps even faster. I share the concern of @iansu - the project officially don't use the cra prefix anywhere (?) but on the other hand. Almost everyone just calls the project cra for short. Is "react-app-" to generic? (Clashes with react-app-rewired which is not a template) |
@andriijas, we could use a different prefix, but I know a lot of the community use the CRA acronym. I think it's safe to use... This is the best we have I think... |
80e40df
to
b2a869e
Compare
@mrmckeb you are right. Let's just acknowledge the community acronym. Excellent work with this! Can't wait to see what kinds of templates the community will bring. Bootstrapping new react apps in 2020 will literally get you productive in seconds! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small comments :)
if (useTypeScript) { | ||
console.log( | ||
chalk.yellow( | ||
'The --typescript option has been deprecated and will be removed in a future release.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this message link to the templates documentation? Same with https://github.com/facebook/create-react-app/pull/7716/files#diff-f9867c1e09ced1328f2ccdac4afac4a5R88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could do, but I can't create short links. @iansu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have our new shortlinks domain name set up yet. Maybe just make a TODO and we'll add it before the final release.
b2a869e
to
d81d7c4
Compare
d81d7c4
to
83c226a
Compare
if (useTypeScript) { | ||
console.log( | ||
chalk.yellow( | ||
'The --typescript option has been deprecated and will be removed in a future release.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have our new shortlinks domain name set up yet. Maybe just make a TODO and we'll add it before the final release.
@@ -0,0 +1,13 @@ | |||
body { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this file for? Was it in the original template?
@@ -1,78 +1,79 @@ | |||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole file shows as changed because it previously had windows line endings. 😞
Something has gone horribly wrong with our CI here. Appveyor is in the list of checks and we got rid of that like a year ago. |
Thanks! ❤️ |
This is an updated version of #6514.
TODO: