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

Proposal: 'configurator' field in package.json #215

Closed
gaearon opened this issue Jul 26, 2016 · 20 comments
Closed

Proposal: 'configurator' field in package.json #215

gaearon opened this issue Jul 26, 2016 · 20 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

tl;dr

Add a field to package.json that tells ESLint, Flow, other tools, where to look for configs.
Useful for zero-conf opinionated packages like create-react-app and standard.

  // ESLint will look for react-scripts/config/.eslintrc
  // Flow will look for react-scripts/config/.flowconfig
  "configurator": "react-scripts"

As a community, we’ve mostly solved the problem of nested, extendable configuration. ESLint does this really well, Babel also does this.

However, I think that this project gaining 4k stars in 4 days proves that there is a market for non-configurable opinionated tools hiding dependencies from the user. I’d like to give props to standard for being one of the first tools demonstrating this is a viable model (it doesn’t matter whether I agree with its choices).

The problem I‘m starting to see is that many amazing tools, like ESLint and Flow, don’t currently play very well with this approach out of the box.

Problem

ESLint

ESLint is very flexible but if you want IDE integration, you need to tell it where your config is.
This is why our otherwise zeroconf package.json now has to look like this:

{
  "name": "my-app",
  "version": "0.0.1",
  "private": true,
  "devDependencies": {
    "react-scripts": "0.1.0"
  },
  "dependencies": {
    "react": "^15.2.1",
    "react-dom": "^15.2.1"
  },
  "scripts": {
    "start": "react-scripts start",
    "build": "react-scripts build",
    "eject": "react-scripts eject"
  },
  "eslintConfig": {
    "extends": "./node_modules/react-scripts/config/eslint.js" // 😁
  }
}

Yes, one extra field is not a high price, but it is already a bit frustrating that we have to hardcode ESLint there. We would like to treat it as an implementation detail, to the extent that it is reasonable. (e.g. eslint-ignore comments are fine, but putting paths to configs into user’s package.json is annoying)

Also, it doesn’t even solve the problem completely because ESLint will look for plugins relative to the project root, so any transitive plugin dependencies won’t be discovered with npm 2 (or in some cases, admittedly not likely with our tool, with npm 3). You can find the discussion about this in eslint/eslint#3458.

Flow

Flow poses a similar problem for us, although in a more unfortunate way. We can’t even tell Flow to look at a different config. When you run flow init, it creates .flowconfig in the project root, and that’s it. This is reasonable if the user plans to change it (which seems necessary for lib integrations), but we can’t even influence the default generated config in any way. And unfortunately, for a number of reasons, the default generated config won’t work for create-react-app.

If you want Flow to work, you need to replace the config generated by flow init with this one:

[libs]
./node_modules/fbjs/flow/lib

[options]
esproposal.class_static_fields=enable
esproposal.class_instance_fields=enable

module.name_mapper='^\(.*\)\.css$' -> 'react-scripts/config/flow/css'
module.name_mapper='^\(.*\)\.\(jpg\|png\|gif\|eot\|svg\|ttf\|woff\|woff2\|mp4\|webm\)$' -> 'react-scripts/config/flow/file'

suppress_type=$FlowIssue
suppress_type=$FlowFixMe

And we don’t want to do this automatically because:

  • Many people don’t currently use Flow.
  • Our philosophy is to not generate a ton of configs in the user folder.
  • We want the config to be compatible with the current version of react-scripts, so if we generate Flow config before the person actually starts using Flow, by the time they start using it, their checks might fail because something changed on our side.

Solution?

I don’t know if there is a nice solution to this, but here is my proposal. This is how I want package.json to look:

{
  "name": "my-app",
  "version": "0.0.1",
  "private": true,
  "devDependencies": {
    "react-scripts": "0.1.0"
  },
  "dependencies": {
    "react": "^15.2.1",
    "react-dom": "^15.2.1"
  },
  "scripts": {
    "start": "react-scripts start",
    "build": "react-scripts build",
    "eject": "react-scripts eject"
  },
  "configurator": "react-scripts" // 😀
}

ESLint would then use react-scripts/config as the base directory for its regular config/plugin search mechanism. So it would look for .eslintrc, package.json, etc, in that directory, and resolve plugin modules relative to it. Since ESLint wants to stay configurable, it will also look for overrides in the current folder (and use its regular override mechanism), but this would just add configurator as a first-class citizen for providing configuration.

Similarly, Flow would look into that project when running flow init, and if it finds .flowconfig in react-scripts/config, it would use that instead of its default config. This gives us a way to own the default configuration even if the user is on their own. In the future, Flow could support merging ${configurator}/config/.flowconfig with local .flowconfig—similar to how ESLint would work if it implemented this feature.

This would benefit an ecosystem by fostering development of opinionated presets of preconfigured tools. If you’re a tool, honor configurator in package.json and try reading configuration from ${configurator}/config in addition to your normal lookup paths. If you’re a preset author, you can fork react-scripts or any other configurator, make some changes to it, and keep it simple to adopt your preset: just swapping configurator is all end users need to do.

What do you think? Am I missing something?

@joshmanders
Copy link

One thing I know about npm is it has a config option1 inside the package.json that almost all of these modules SHOULD use, but don't. They create their own instance on the base object.

Maybe extend this, to check if $npm_package_config env var to see if it's a configurator project and handle that way.

I do like the idea of this though.

@gaearon
Copy link
Contributor Author

gaearon commented Jul 26, 2016

Yeah, it serves a different purpose though (we could probably use it for stuff like choosing ports in the future). What I suggest here is a way to “redirect” tool-specific configuration. You can use config and configurator side by side. I see how the naming could be a little confusing though but I don’t think it’s a big deal, and better naming is welcome!

@joshmanders
Copy link

I really like the idea though, I use package.json to set a lot of options for all these tools because I hate littering my directory with dot, yaml, and json files just for each tool I use. I'd totally help with pull requests to project implementing a configurator loader.

@vjeux
Copy link
Contributor

vjeux commented Jul 26, 2016

There's already a thousand ways that all those tools can figure out where to find the configuration, it's really unclear to me that adding yet another one is the right solution. Also, adding a hardcoded path to the config that the tools can read means that anybody can also read it and it's not a blackbox anymore.

Proposal instead:

Have the IDEs read package.json and if there's a script called eslint, then use it instead of the global eslint.

Then, we would add an opaque script that calls eslint with --config=opaquepath/eslintrc

@ForbesLindesay
Copy link
Contributor

@vjeux I don't think that solves the problem of the proliferation of information in the package.json that the user shouldn't need to worry about. They will at a minimum end up seeing a script for eslint and a script for flow. It also prevents IDE based tooling from doing any kind of inspecting of what your config is (e.g. IDEs could detect what indentation rules you use from eslint to auto-configure the behaviour of tab).

@gaearon
Copy link
Contributor Author

gaearon commented Jul 26, 2016

Have the IDEs read package.json and if there's a script called eslint, then use it instead of the global eslint.

I like the simplicity of this. I agree with @ForbesLindesay this doesn’t solve the config proliferation problem though so I‘m not sure it’s much better than the status quo. This is more or less what we’re doing anyway with eslintConfig field.

Also, this doesn’t solve the Flow issue, does it?

@ForbesLindesay
Copy link
Contributor

I think the idea would be that you would run react-scripts flow and that would cause us to call flow with whatever arguments flow needed in order to have the correctly configured options. I'm not sure that would work very well though, especially with all flow's persistent server architecture quirks.

@vjeux
Copy link
Contributor

vjeux commented Jul 26, 2016

I'm not sure I understand the issue with flow being a server. If the IDE uses it as a server it is going to call it once with the options. If it uses it as a standalone tool it's going to call it multiple times. Having a wrapper that sets the config flag shouldn't change anything?

The goal here is to tell the IDE/CI... how to invoke the tools properly configured without letting them introspect the actual configuration. This way they don't end up relying on specifics.

@gaearon
Copy link
Contributor Author

gaearon commented Jul 26, 2016

The downside with #215 (comment) is that every IDE/plugin would need to implement this. So, for each of N tools (ESLint, Flow), there would be M plugins (Atom, Sublime), and NxM integrations that would need to do it this way (ESLint for Atom, ESLint for Sublime, Flow for Atom, ...).

Whereas if we do this on the tool level as suggested in #215 (comment), we only need the N tools to integrate this. IDEs already call the tools with no arguments and rely on the tools to know how to discover configuration.

@taion
Copy link

taion commented Jul 26, 2016

Would it? Don't those tools just effectively run ESLint? If the eslint binary can handle it, why would it be any extra work for IDEs?

@gaearon
Copy link
Contributor Author

gaearon commented Jul 26, 2016

Would it? Don't those tools just effectively run ESLint? If the eslint binary can handle it, why would it be any extra work for IDEs?

Sorry, I meant @vjeux’s proposal. Edited the message.

@saschwarz
Copy link

A different approach is used in Python packaging with a file in the package root named 'setup.cfg' into which tools can have have a section (or not) with their configurations: https://docs.python.org/3/distutils/configfile.html

When the tool is invoked it reads it's config from the setup.cfg.

@gaearon
Copy link
Contributor Author

gaearon commented Jul 26, 2016

@saschwarz This is very similar to how package.json configuration works now (you can find an example with eslintConfig in the post). But it means that user’s package.json gets bloated with configuration links that don’t matter if you use a zeroconf tool like create-react-app.

@ForbesLindesay
Copy link
Contributor

@vjeux The issue is that flow is way more complicated than just "run it once" or "run it many times". There is flo which is the server/demon part and flow which is basically just a client for flo, but I think the IDEs might call directly into flo. I'm not really certain of any of this, it just strikes me that the npm scripts approach could get very complex very fast, and as @gaearon suggests, all m * n plugins for IDEs and editors need to deal with that complexity, not just the n different linters and build tools.

@saschwarz
Copy link

@gaearon Sorry I wasn't being clear. I meant it as an additional file just for tool configs; the package.json would hold everything else: dependencies, user scripts, etc. One advantage of a separate tooling config file is it is likely to be the same across multiple projects for a user/organization.

@johnrhampton
Copy link

I really dig the concept of abstracting away the build configurations. Copy/pasting scripts to handle bundling or workflow management has been plaguing us since the days of grunt and gulp tasks. react-scripts is a huge step in the right direction. Great stuff guys.

Would it make sense to allow configurator to accept an object of tool directories? This would allow users to stay with react-scripts where needed while easily mixing with other presets.

This may be unnecessary complexity, as default configs can still be used, but wanted to see what you guys thought.

Something like -

{
  "name": "my-app",
  "version": "0.0.1",
  "private": true,
  "devDependencies": {
    "react-scripts": "0.1.0",
    "other-scripts": "0.1.0"
  },
  "dependencies": {...},
  "scripts": {...},
  "configurator": { // 😀
    "flow": "react-scripts", 
    "eslint": "other-scripts"
  }
}

@hnordt
Copy link
Contributor

hnordt commented Jul 26, 2016

@gaearon why not use npm config? https://docs.npmjs.com/files/package.json#config

{
 "name": "x",
 "version": "y",
 "config": {
   "eslint": "react-scripts",
   "flow": "react-scripts"
 }
}

@jamiebuilds
Copy link

This feels like an oversimplification of the complexity of how tools interact with one another.

There is a place for extending configuration in just about every tool in existence, and the way those need to work vary between tool to tool.

If you are to avoid wrapping every tool in one of those gulp/grunt-* packages then there needs to be an open ended solution on how to extend config dynamically.

@taion
Copy link

taion commented Jul 27, 2016

I agree with @thejameskyle there.

This seems like a potentially dangerous attempt to generalize from n = 1.

It's worth at least waiting to see how things pan out before promulgating a new standard that multiple tooling vendors would be expected to follow.

@gaearon
Copy link
Contributor Author

gaearon commented Jul 27, 2016

Good points. I think the problem is still worth exploring but I'd like to hear more alternative solutions for delegating configuration.

@gaearon gaearon closed this as completed Jul 27, 2016
@lock lock bot locked and limited conversation to collaborators Jan 23, 2019
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