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.

Sign up for GitHub

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

Jump to bottom

Add scripts support to templates #7989

Merged
merged 1 commit into from
Nov 19, 2019
Merged

Add scripts support to templates #7989

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 36 additions & 16 deletions packages/react-scripts/scripts/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

if (templateName) {
templateJsonPath = path.join(templatePath, 'template.json');
} else {
// TODO: Remove support for this in v4.
templateJsonPath = path.join(appPath, '.template.dependencies.json');
}

let templateJson = {};
if (fs.existsSync(templateJsonPath)) {
templateJson = require(templateJsonPath);
}

// Copy over some of the devDependencies
appPackage.dependencies = appPackage.dependencies || {};

// Setup the script rules
appPackage.scripts = {
start: 'react-scripts start',
build: 'react-scripts build',
test: 'react-scripts test',
eject: 'react-scripts eject',
};
const templateScripts = templateJson.scripts || {};
appPackage.scripts = Object.assign(
{
start: 'react-scripts start',
build: 'react-scripts build',
test: 'react-scripts test',
eject: 'react-scripts eject',
},
templateScripts
);

// Update scripts for Yarn users
if (useYarn) {
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.

}),
{}
);
}

// Setup the eslint config
appPackage.eslintConfig = {
Expand Down Expand Up @@ -196,21 +224,13 @@ module.exports = function(
}

// Install additional template dependencies, if present
let templateJsonPath;
if (templateName) {
templateJsonPath = path.join(templatePath, 'template.json');
} else {
templateJsonPath = path.join(appPath, '.template.dependencies.json');
}

if (fs.existsSync(templateJsonPath)) {
const templateDependencies = require(templateJsonPath).dependencies;
const templateDependencies = templateJson.dependencies;
if (templateDependencies) {
args = args.concat(
Object.keys(templateDependencies).map(key => {
return `${key}@${templateDependencies[key]}`;
})
);
fs.unlinkSync(templateJsonPath);
mrmckeb marked this conversation as resolved.
Show resolved Hide resolved
}

// Install react and react-dom for backward compatibility with old CRA cli
Expand Down