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 for any package.json keys (#8082) #8219

Merged
merged 4 commits into from Jan 12, 2020

Conversation

tomvalorsa
Copy link
Contributor

This PR adds support for any package.json keys in a template.json file. It uses a blacklist of specific keys that shouldn’t be touched, and explicitly lists which keys will be merged with existing config - any other keys will be replaced as discussed in #8082.

Some notes:

  • explicit merging of "scripts" and "dependencies" kept intact
  • templatePackage keys that aren’t blacklisted or specified as merge keys will be added to appPackage
    • this also replaces existing entries in appPackage with the new values specified in templatePackage
  • stock templates have been updated to reflect the change in template.json, "kitchensink" fixture also updated
  • docs have been updated

Some thoughts:

  • some of the names used feel a bit chunky, open to suggestions!
  • I chose "browserslist" as the example for the docs page based on fix: add template support for homepage, browserslist properties #8209 but can change if there’s a clearer example (a real example felt better than "some key": "some value")
  • I added a couple of deprecation notes as "TODOs" as a I saw this convention elsewhere in react-scripts/init.js. These are based on the plan to deprecate the old template style here. Let me know if this should live somewhere else, or if there’s a specific format you follow
  • not sure how you manage changelogs but if you’re using issue titles the related issue doesn’t really represent this PR anymore so it should be changed

Test plan:

I created a custom template that was a direct copy of cra-template, and then tested different scenarios. There are various tests split across different branches (one per branch). N.B. the changes here are just related to template.json on each of these branches. Quick summary:

  • master - direct copy of cra-template
  • test-01-package-key - added a different dependency to package.dependencies to ensure it was installed correctly
  • test-02-scripts-key - added scripts to add and replace to package.scripts
  • test-03-old-deps-scripts - added deps and scripts in the old format to ensure it still works
  • test-04-old-and-new-deps-and-scripts - added both new and old formats to ensure the new format takes precedence
  • test-05-blacklist-keys - added all of the blacklist keys to ensure they aren’t copied over
  • test-06-custom-keys-to-replace-extend - added some custom, non-blacklisted keys to ensure they are copied over
  • test-07-empty-old-config - tried with an empty config
  • test-08-empty-new-config - tried with an empty package
  • I also ran with cra-template-typescript

All of the above worked as expected, I’m happy to run again and provide screenshots if needed. I ran these tests by cloning my custom template and running the following command, changing branches in the custom template dir as necessary:

npx create-react-app test-app --scripts-version=file:path/to/react-scripts --template=file:path/to/custom/template

I wasn’t sure how to run other tests in the repo so I’m pushing this up now to see if CI passes.

Any feedback would be greatly appreciated :)

@facebook-github-bot
Copy link

Hi tomvalorsa! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 26, 2019

Thanks @tomvalorsa, I'll try to get through this one tomorrow - sorry, Christmas is getting in the way. Looks good at a first glance.

Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

This looks great @tomvalorsa, only a few suggested changes to copy and I think we should increase the scope of the blacklist for now.

Sorry again for the delay, I'm mostly back from holidays now.

"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.

@@ -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 👍

}
}
```

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!

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.

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.

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.

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 3, 2020

This looks good to me, @ianschmitz can I get your thoughts?

Test failures seem unrelated, I'm not sure what's happening there.

@mrmckeb mrmckeb added this to the 3.4 milestone Jan 3, 2020
@mrmckeb mrmckeb self-assigned this Jan 3, 2020
@jimthedev
Copy link

Looking good so far. @ianschmitz @mrmckeb anything I can do to help push this over the finish line?

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 9, 2020

Just waiting for a second review @tomvalorsa :)

Maybe from @ianschmitz or @heyimalex.

Copy link
Contributor

@heyimalex heyimalex left a comment

Choose a reason for hiding this comment

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

Other than that minor nit, this looks great! Thanks for the really well thought out PR.

// replacing any existing entries.
const templatePackageToReplace = Object.keys(templatePackage).filter(key => {
return (
templatePackageBlacklist.indexOf(key) === -1 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this and the following line to use includes instead?

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 👍

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 12, 2020

Thanks @tomvalorsa, merged. I'm not sure when 3.4 will go out, but this will be in for sure.

@mrmckeb mrmckeb merged commit 94932be into facebook:master Jan 12, 2020
@tomvalorsa
Copy link
Contributor Author

Great! Thanks for the help and feedback everyone

@jimthedev
Copy link

Awesome! Does CRA get experimental hash npm releases by chance like react does?

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 12, 2020

Sorry... but no :( It's on our TODO list, we know it'd be helpful to some users.

@jimthedev
Copy link

No worries. Appreciate all your work on this.

@lock lock bot locked and limited conversation to collaborators Jan 17, 2020
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.

None yet

5 participants