-
-
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 scripts support to templates #7989
Conversation
@@ -105,16 +105,44 @@ module.exports = function( | |||
'..' | |||
); | |||
|
|||
let templateJsonPath; |
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 code was previously lower.
appPackage.scripts = Object.entries(appPackage.scripts).reduce( | ||
(acc, [key, value]) => ({ | ||
...acc, | ||
[key]: value.replace(/(npm run |npm )/, 'yarn '), |
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.
Replace any npm
commands for Yarn users. This is the same logic we use for replacing those commands in the README file.
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.
Do we do this for our scripts currently?
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.
Not sure that we need to be too smart about this.
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.
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.
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 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.
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.
npm run build-dependency && npm run build
will work on windows without WSL. Is that what you meant?
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.
OK, that's good to know - I searched briefly and got mixed results. Thanks @ianschmitz.
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 (viapackage.json
) to3.3.0
.