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

Add template support for any package.json keys (#8082) #8219

Merged
merged 4 commits into from
Jan 12, 2020
Merged

Add template support for any package.json keys (#8082) #8219

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e2d7111
Add template support for any package.json keys (#8082)
tomvalorsa Dec 22, 2019
3b821b0
Update custom template documentation, extend template package blackli…
tomvalorsa Jan 3, 2020
9bfa838
Remove 'homepage' from template blacklist (#8082)
tomvalorsa Jan 4, 2020
000d230
Change indexOf() to includes()
tomvalorsa Jan 9, 2020
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
24 changes: 17 additions & 7 deletions docusaurus/docs/custom-templates.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,18 +58,28 @@ You can add whatever files you want in here, but you must have at least the file

### The `template.json` file

This is where you can define dependencies (only dependencies are supported for now), as well as any custom scripts that your template relies on.
This is the configuration file for your template. It allows you to define information that should become a part of the generated project's `package.json` file, such as dependencies (only dependencies are supported for now) and any custom scripts that your template relies on. It should be structured as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This is the configuration file for your template. It allows you to define information that should become a part of the generated project's `package.json` file, such as dependencies (only dependencies are supported for now) and any custom scripts that your template relies on. It should be structured as follows:
This is the configuration file for your template. As this is a new feature, more options will be added over time. For now, only a `package` key is supported.
The `package` key lets you provide any keys/values that you want added to the new project's `package.json`, such as dependencies (only dependencies are supported for now) and any custom scripts that your template relies on.
Below is an example `template.json` file:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍


```json
{
"dependencies": {
"serve": "^11.2.0"
},
"scripts": {
"serve": "serve -s build",
"build-and-serve": "npm run build && npm run serve"
"package": {
"dependencies": {
"serve": "^11.2.0"
},
"scripts": {
"serve": "serve -s build",
"build-and-serve": "npm run build && npm run serve"
},
"browserslist": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be simpler to add an ESLint config here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I've used the example from the eslint-config-react-app docs to ensure it doesn't conflict with something already covered in the react-app config.

"defaults",
"not IE 11",
"not IE_Mob 11",
"maintained node versions",
]
}
}
```

Any values you add for `"dependencies"` and `"scripts"` will be merged with the values used in the initialisation process of `react-scripts`. Any other information you add to `"package"` will be added to the generated project's `package.json` file, replacing any existing values associated with those keys.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this, but maybe something like the below?

Suggested change
Any values you add for `"dependencies"` and `"scripts"` will be merged with the values used in the initialisation process of `react-scripts`. Any other information you add to `"package"` will be added to the generated project's `package.json` file, replacing any existing values associated with those keys.
Any values you add for `"dependencies"` and `"scripts"` will be merged with the Create React App defaults. Values for any other keys will be used as-is, replacing any matching Create React App defaults.

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've updated this to your suggestion above. I tried to think of a few different ways to phrase it but they were a bit verbose - this seems to be the most concise. The message is essentially "these keys will be merged with defaults, the rest will be added and replace existing defaults" so I think the less words the better!


For convenience, we always replace `npm run` with `yarn` in your custom `"scripts"`, as well as in your `README` when projects are initialized with yarn.
20 changes: 11 additions & 9 deletions packages/cra-template-typescript/template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
{
"dependencies": {
"@testing-library/react": "^9.3.2",
"@testing-library/jest-dom": "^4.2.4",
"@testing-library/user-event": "^7.1.2",
"@types/node": "^12.0.0",
"@types/react": "^16.9.0",
"@types/react-dom": "^16.9.0",
"@types/jest": "^24.0.0",
"typescript": "~3.7.2"
"package": {
"dependencies": {
"@testing-library/react": "^9.3.2",
"@testing-library/jest-dom": "^4.2.4",
"@testing-library/user-event": "^7.1.2",
"@types/node": "^12.0.0",
"@types/react": "^16.9.0",
"@types/react-dom": "^16.9.0",
"@types/jest": "^24.0.0",
"typescript": "~3.7.2"
}
}
}
10 changes: 6 additions & 4 deletions packages/cra-template/template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
{
"dependencies": {
"@testing-library/react": "^9.3.2",
"@testing-library/jest-dom": "^4.2.4",
"@testing-library/user-event": "^7.1.2"
"package": {
"dependencies": {
"@testing-library/react": "^9.3.2",
"@testing-library/jest-dom": "^4.2.4",
"@testing-library/user-event": "^7.1.2"
}
}
}
16 changes: 9 additions & 7 deletions packages/react-scripts/fixtures/kitchensink/template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
{
"dependencies": {
"bootstrap": "4.3.1",
"jest": "24.9.0",
"node-sass": "4.12.0",
"normalize.css": "7.0.0",
"prop-types": "15.7.2",
"test-integrity": "2.0.1"
"package": {
"dependencies": {
"bootstrap": "4.3.1",
"jest": "24.9.0",
"node-sass": "4.12.0",
"normalize.css": "7.0.0",
"prop-types": "15.7.2",
"test-integrity": "2.0.1"
}
}
}
36 changes: 34 additions & 2 deletions packages/react-scripts/scripts/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,37 @@ module.exports = function(
templateJson = require(templateJsonPath);
}

const templatePackage = templateJson.package || {};

// Keys to ignore in templatePackage
const templatePackageBlacklist = [
'devDependencies',
'peerDependencies',
'name',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we blacklist all keys listed here for now - except for config and engines:
https://docs.npmjs.com/files/package.json

We can then enable them on a case-by-case basis, unless you can think of some more that might be immediately useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing jumps out at the moment so I've added them all in to the blacklist, except for config and engines as requested.

Copy link

Choose a reason for hiding this comment

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

Please also remember I asked for a few to be immediately whitelisted here: #8209

I closed my PR in favor of this one but specifically keeping homepage off the blacklist would be great so that we can use homepage: "." which is a webpack recommendation in certain scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies @jimthedev, oversight on my part - I've updated this PR to remove homepage from the blacklist so that your PR is covered.

Regarding whitelisting other keys, there is only a blacklist so any keys used that are not in the blacklist (e.g. browserslist) will be copied over to the output package.json. Here is a test example of a template.json file with the browserslist field that was run successfully. Instructions to run this are in the initial PR description if you're interested.

Copy link

Choose a reason for hiding this comment

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

Thanks much @tomvalorsa. Makes sense. Appreciate you working on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for being on top of this @tomvalorsa. I'll try to get someone to review ASAP.

];

// Keys from templatePackage that will be merged with appPackage
const templatePackageToMerge = [
'dependencies',
'scripts',
];

// Keys from templatePackage that will be added to appPackage,
// replacing any existing entries.
const templatePackageToReplace = Object.keys(templatePackage)
.filter(key => {
return (
templatePackageBlacklist.indexOf(key) === -1 &&
templatePackageToMerge.indexOf(key) === -1
);
});

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

// Setup the script rules
const templateScripts = templateJson.scripts || {};
// TODO: deprecate 'scripts' key directly on templateJson
const templateScripts = templatePackage.scripts || templateJson.scripts || {};
appPackage.scripts = Object.assign(
{
start: 'react-scripts start',
Expand Down Expand Up @@ -152,6 +178,11 @@ module.exports = function(
// Setup the browsers list
appPackage.browserslist = defaultBrowsers;

// Add templatePackage keys/values to appPackage, replacing existing entries
templatePackageToReplace.forEach(key => {
appPackage[key] = templatePackage[key];
});

fs.writeFileSync(
path.join(appPath, 'package.json'),
JSON.stringify(appPackage, null, 2) + os.EOL
Expand Down Expand Up @@ -221,7 +252,8 @@ module.exports = function(
}

// Install additional template dependencies, if present
const templateDependencies = templateJson.dependencies;
// TODO: deprecate 'dependencies' key directly on templateJson
const templateDependencies = templatePackage.dependencies || templateJson.dependencies;
if (templateDependencies) {
args = args.concat(
Object.keys(templateDependencies).map(key => {
Expand Down