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

Add template support #6514

Closed
wants to merge 3 commits into from
Closed

Add template support #6514

wants to merge 3 commits into from

Conversation

mrmckeb
Copy link
Contributor

@mrmckeb mrmckeb commented Feb 25, 2019

This is a work in progress. Do not merge.

This has become a little more complicated than I first envisioned. Right now, I'm still working through the init part of this. The plan is as follows:

  • react-scripts and create-react-app-template-* are installed as dependencies. Either or both of those can be defined via CLI.
    • Default is current default template.
    • Although the supported scripts version (range) is included in the template's peerDependencies, this is not used during install.
  • Upon react-scripts init, the template is copied from template package, and peerDependencies are installed from that package (excluding the scripts-version). That package is then removed, as it no longer has any value.
    • When using non-standard scripts, or a scripts version that is below the peerDependencies of the template, appropriate warnings will be shown - but the user can still continue. This, importantly, allows custom scripts versions to work with the template.
    • Optionally, the scripts version range could be in engines... but that doesn't feel clean.

The typescript flag is deprecated under this process.

@mrmckeb
Copy link
Contributor Author

mrmckeb commented Mar 3, 2019

OK, so I need to work on tests for this - and make sure it all works correctly - but the idea is in place.

The biggest issue I see with this implementation is that a) the template may require a different scripts version than the one the user set, and b) as the template is handled by react-scripts, we can only warn a user when scripts versions don't match - when perhaps we should just install the scripts package requested by the template.

@iansu iansu mentioned this pull request Mar 5, 2019
@Ugzuzg
Copy link

Ugzuzg commented Mar 7, 2019

Would be awesome to have the ability to add custom scripts to the package.json. Say, my template has internationalisation, and I want to add extraction script to the project.

@iansu iansu mentioned this pull request Mar 14, 2019
@mrmckeb
Copy link
Contributor Author

mrmckeb commented Mar 18, 2019

This can be tested with:

node ./create-react-app/packages/create-react-app test-project \
  --template file:./create-react-app/packages/create-react-app-template/ \
  --scripts-version file:./create-react-app/packages/react-scripts/

@mrmckeb mrmckeb marked this pull request as ready for review March 19, 2019 04:43
@nartoland
Copy link

Ok

@gc
Copy link

gc commented Mar 20, 2019

After cloning your branch, and running your exact command, I get this error:

error An unexpected error occurred: "https://registry.yarnpkg.com/create-react-app-template-file;./create-react-app/packages/create-react-app-template/: Reques
t \"https://registry.yarnpkg.com/create-react-app-template-file;./create-react-app/packages/create-react-app-template/\" returned a 405".
info If you think this is a bug, please open a bug report with the information provided in "C:\\Dev\\test-project\\yarn-error.log".
Aborting installation.
  yarnpkg add --exact react react-dom create-react-app-template-file;.\create-react-app\packages\create-react-app-template\ file;.\create-react-app\packages\re
act-scripts\ --cwd C:\Dev\test-project has failed.

Yarn v1.12.3
Node v11.6.0
OS win32 x64

EDIT: I tried debugging this but I'm not really sure whats wrong or causing it, a couple observations I made are:

  • : is being changed into ; for some reason, I can manually .replace this to get past that issue
  • it attempts to install file:./... for the template and scripts, if i remove the file: in this, it seems to get further without crashing.
  • if that test cmd works fine for you, then perhaps its an issue with windows? assuming you're using linux/macOS

@mrmckeb
Copy link
Contributor Author

mrmckeb commented Mar 25, 2019

I'm working on Ubuntu (WSL actually), but it may be an issue we need to look into.

That being said, the file:.. resolution` if for testing this PR and is actually passing through to npm/Yarn.

@Ugzuzg
Copy link

Ugzuzg commented Aug 6, 2019

Any further work on this?

@mrmckeb
Copy link
Contributor Author

mrmckeb commented Aug 8, 2019

Hi @Ugzuzg, this is a 3.2 goal now - I'll be restarting work on this next week.

We had some more urgent matters and wanted to think about this a little more.

Perhaps you can share a little more about what you'd like to see from templates?

@mrmckeb mrmckeb added this to the 3.2 milestone Aug 8, 2019
@Ugzuzg
Copy link

Ugzuzg commented Aug 8, 2019

Custom scripts, as mentioned above.

Also would be great to install additional dependencies. This PR achieves custom dependencies installing peerDependencies of a template, I believe? So this case is covered.
The ability to distinguish deps between dependencies and devDependencies isn't necessary, but is welcomed.

Thank you!

@mrmckeb mrmckeb closed this Sep 22, 2019
@mrmckeb mrmckeb mentioned this pull request Sep 22, 2019
4 tasks
@lock lock bot locked and limited conversation to collaborators Sep 27, 2019
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.

7 participants