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 devDependencies for custom templates #8082

Closed
tomvalorsa opened this issue Dec 5, 2019 · 26 comments
Closed

Add devDependencies for custom templates #8082

tomvalorsa opened this issue Dec 5, 2019 · 26 comments

Comments

@tomvalorsa
Copy link
Contributor

Is your proposal related to a problem?

Hey 👋good job on the recent release.

I’m really keen to make a custom template for CRA but want to add dev dependencies. Are there plans to add support for this is the near-ish future? Happy to throw up a PR if you’re busy with other stuff.

I made a fork of CRA a while ago so I could add custom templates but a community-supported effort is much more appealing!

Describe the solution you'd like

Simply adding another key to template.json for "devDependencies".

Where dependencies start to be installed here, there could be a function to wrap the reusable stuff (building up args, spawning process etc.) that could take an dependency type and the relevant slice of template.json.

Describe alternatives you've considered

Nothing at the moment as the template feature is obviously very new.

Additional context

This could potentially open up the possibility of adding peerDependencies too but I haven't really worked with them directly before, so not sure if this is useful/appropriate.

@lzm0x219
Copy link

lzm0x219 commented Dec 6, 2019

I want to support not only devDependencies, but also a complete package.json configuration

@tomvalorsa
Copy link
Contributor Author

Yeah that would be cool. I want to create a template that already has prettier setup with husky hooks, but I realise now devDependencies alone wouldn't cut it.

In the fork I made each of my templates had a template.json equivalent which was merged with the generated app’s package.json. But of course I was only catering to myself. There could be problems with clobbering useful stuff that react-scripts generates…

@lzm0x219
Copy link

lzm0x219 commented Dec 6, 2019

@tomvalorsa I agree with you. I want to overwrite it based on the custom template. It's more flexible.🌝

@reactjser
Copy link
Contributor

It will be more flexible if I could add any fields in template.json, such as husky or lint-staged, not only the devDependencies.🙃

@lzm0x219
Copy link

lzm0x219 commented Dec 6, 2019

@reactjser yeah.

@lukaszfiszer
Copy link
Contributor

@tomvalorsa it is possible to create a template with prettier and husky configured. Just put your config inside prettier.config.js and husky.config.js instead of package.json

@v19i
Copy link

v19i commented Dec 13, 2019

Is anyone working on this? I'm interested in solving this issue.

@lzm0x219
Copy link

@franz899 👌。

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 18, 2019

Hi @tomvalorsa, the CRA team have historically been against having devDependencies in the templates, as most users run tests/etc on the CI anyway, so there's rarely a time where you would only want to install one set of dependencies.

That being said, things change, and we're always open to discussion if people have good reasoning for change.

@lukaszfiszer is right, you can use separate configs for things like husky - but we're open to allowing more keys to be added to the template.json file.

I'd personally be in favour of adding a "additional keys" property, which would allow things like eslint and prettier keys - and would exclude dependency keys (as they would need to be processed). Or we could whitelist config keys - but that would be a slippery slope I think. Maybe we could call the key additionalKeys or packageConfigs?

@mrmckeb mrmckeb added this to the 3.4 milestone Dec 18, 2019
@lzm0x219
Copy link

lzm0x219 commented Dec 18, 2019

All right...

@reactjser
Copy link
Contributor

All right...

Hope for this feature.😃

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 18, 2019

Hey, what's with all the sadness? :) Let's put the question this way:

  1. What would be the benefit of adding devDependencies right now?
  2. If we had to invest time into something, would you rather configs or devDependencies?

This is an honest question, we love your feedback and want to make a product that works well for all of our users (a difficult balance of course).

@lzm0x219
Copy link

@mrmckeb Not sad, I think you are right.😄

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 18, 2019

I'll wait for some more feedback - and any volunteers (optional) - otherwise, I could pick this up over the weekend or next week 🎅.

@lzm0x219
Copy link

@mrmckeb 👌~

@tomvalorsa
Copy link
Contributor Author

Hey @mrmckeb, that makes sense!

To answer your questions above the main goal I have is setting up a project using a template which requires as little extra work as possible, e.g. setting up prettier so that other devs in my team don’t have to. Given this I think the ability to add additional config is more important than devDependencies.

I think something like the "additionalKeys" property for template.json makes a lot of sense, and would make for more flexible templates/configurations. I also agree that having a list of exclusions rather than a whitelist would be better.

I'd like to work on this 🙋‍♂

@lukaszfiszer thanks for the tip - I’ve been adding this config to package.json for so long that I forgot you could split them out!

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 18, 2019

You're up @tomvalorsa! Feel free to ping me on Discord, Spectrum or Twitter if you want to talk through anything. The most important thing (IMO) is that this new key has a blacklist for keys we include by default, or at least handles those.

I'll talk to the rest of the team about this too and see if anyone wants to raise a red flag.

@malept
Copy link

malept commented Dec 18, 2019

👋 I'd like to chime in about devDependencies support. I work on Electron ecosystem tools, and the Electron project recommends that electron is added to devDependencies instead of dependencies. Electron is not considered a production dependency because the app is executed using the Electron binary bundled with the app, not the one that would be in node_modules. I can get into that in more detail if needed, but effectively, if it weren't in devDependencies, you'd have two copies of the Electron binary in a packaged app. And many folks on social media are quick to point out how "large" Electron apps are in general.

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 19, 2019

@tomvalorsa I met with @ianschmitz and @iansu yesterday and we want to take this approach for now.

All package.json keys will move to a package object. The reasoning for this is that we want to keep template.json as a config for templates, we plan for it to contain other logic in future - such as compatible versions, etc.

Example:

{
  "package": {
    "scripts": {},
    "dependencies": {},
    "husky": {},
  }
}

The existing keys will work, but will be deprecated and removed in the next major.

For now, peerDependencies and devDependencies (along with keys like name) should be blacklisted, and we'll open up discussions on how to handle those for the next major. Keys that aren't blacklisted should be accepted.

This doc will also need to be updated as a part of your PR:
https://create-react-app.dev/docs/custom-templates

You would need to document which keys are merged, and which replaced too. I think, apart from dependencies and scripts, most could be replaced... but open to thoughts.

Let me know if you have any concerns, and again, I'm here to help if you have any questions.


Note: PR #8209 was opened to add some of these features. I've closed for now, as it conflicts with the discussed approach - but as mentioned, this work should allow any package.json keys, excluding those blacklisted.

CC @jimthedev who may want to chime in with further thoughts.

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 19, 2019

@malept we haven't officially supported electron to date, but if you'd like to open a separate ticket and tag me in it - we can discuss there. I'd love to understand more.

@jimthedev
Copy link

Thanks @mrmckeb. I’m in favor of the package key. It is more robust. I would just mention that the sooner the better since templates being created now would not be following that and thus it seems to be a breaking change that gets worse as time goes on. :)

@tomvalorsa
Copy link
Contributor Author

@mrmckeb makes sense, thanks for supporting info. I will get started on this properly today and will reach out if I have any questions.

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 19, 2019

No problem @jimthedev, I'll work with the team to ship this as soon as it's ready.

Thanks for the update @tomvalorsa!

@lukaszfiszer
Copy link
Contributor

Please note that packages like husky will not correctly add hooks to the local repo when being installed as template dependency, as described in #8280

@apples
Copy link

apples commented Jan 22, 2020

@mrmckeb I'm also in need of devDependencies for creating a template for Electron apps.

I don't think it really deserves a separate issue, since it doesn't require officially supporting Electron in any way.

In addition to what @malept has said: The most common packager for Electron apps, electron-builder, has an explicit hard-coded check to ensure that electron (and electron-builder itself) is not in dependencies. This means electron must be in devDependencies.

Allowing devDependencies would trivially fix this, and I don't see how allowing it would cause any problems. Since devDependencies is not used by any other part of CRA (at least to my knowledge, please correct me if I'm mistaken), it doesn't even need any special treatment here.

The only other option is to not use CRA at all, which is what my team is currently doing.

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 22, 2020

@apples This issue is meant to be closed sorry, this was an oversight.

I'd ask that you create a new issue - and detail anything else that would help with electron support. We can then have a discussion specific to this, without alerting the others in this thread.

I'm not opposed to allowing it for Electron apps, we just need to discuss with other team members.

@mrmckeb mrmckeb closed this as completed Jan 22, 2020
@lock lock bot locked and limited conversation to collaborators Jan 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants