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 scripts support to templates #7989

Merged
merged 1 commit into from
Nov 19, 2019
Merged

Add scripts support to templates #7989

merged 1 commit into from
Nov 19, 2019

Conversation

mrmckeb
Copy link
Contributor

@mrmckeb mrmckeb commented Nov 16, 2019

To test, add a "scripts" key to any template.json file, and use that template.

This code merges keys in, allowing template authors to add or replace the default scripts.


Example command below. While testing, you may need to bump the react-scripts version (via package.json) to 3.3.0.

node ./create-react-app/packages/create-react-app template-test \                                                                                                                                                                                                           feat/template-scripts
  --template file:./create-react-app/packages/cra-template-typescript/ \
  --scripts-version file:./create-react-app/packages/react-scripts/

@@ -105,16 +105,44 @@ module.exports = function(
'..'
);

let templateJsonPath;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was previously lower.

appPackage.scripts = Object.entries(appPackage.scripts).reduce(
(acc, [key, value]) => ({
...acc,
[key]: value.replace(/(npm run |npm )/, 'yarn '),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace any npm commands for Yarn users. This is the same logic we use for replacing those commands in the README file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we do this for our scripts currently?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that we need to be too smart about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but I think this is a "nice to have" - as we're doing the same thing for the README in the same file.

It creates less confusion for people new to CRA too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should clarify, we don't need this for our scripts - this is to ensure that scripts created by others are normalised - as we do with README files. This allows people to create scripts with commands like the below:

"scripts": {
  "build-dependency": "some-script",
  "compile": "npm run build-dependency && npm run build",
}

We should also consider (in future) normalising across operating systems too. For example, you can't do yarn build && yarn test on Windows (without WSL). But that's too much at this stage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm run build-dependency && npm run build will work on windows without WSL. Is that what you meant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that's good to know - I searched briefly and got mixed results. Thanks @ianschmitz.

@mrmckeb mrmckeb added this to the 3.3 milestone Nov 18, 2019
@mrmckeb mrmckeb merged commit 23d5776 into master Nov 19, 2019
@mrmckeb mrmckeb deleted the feat/template-scripts branch November 19, 2019 10:14
MOZGIII pushed a commit to MOZGIII/create-react-app that referenced this pull request Nov 21, 2019
@lock lock bot locked and limited conversation to collaborators Nov 24, 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.

5 participants