search

GoogleChrome / lighthouse-ci

Automate running Lighthouse for every commit, viewing the changes, and preventing regressions
Apache License 2.0
6.4k stars 641 forks source link

thoughts on configuration #48

Closed connorjclark closed 4 years ago

connorjclark commented 4 years ago

existing work

typescript doesn't have named a CLI option for setting the config file, but their config is named tsconfig.json

eslint uses --config, although it does have --no-eslintrc. allowed config names are many: image

babel doesn't have any way to set a config from the CLI, but it does have --no-babelrc. allowed config names are: image

thoughts

sorted by order of how strongly i feel

--rc-file should be --config

It seems to just be a config, so shouldn't it be called that? I didn't find any tools that name their config option like this.

remove ability to customize config path

I think babel does this to simplify compiling across project boundaries. That doesn't apply much here, but I wonder if there is any purpose to have multiple config files for LHCI? We could consider removing any option to configure, and add it in later if demand is there. If we do this, I'd recommend the file be called .lhciconfig.json (or .lhcirc.json)

allow .js

right now just .json is allowed. this makes sense for now, and we should definitely defer this. just calling it out now

configure via package.json

ditto above section

nit on rc

rc stands for run commands*, and so I'd think files called rc should be something you must run (shell, node, etc). I wouldn't consider a json file to be a executable file. I see that eslint uses eslintrc.js and eslintrc.json (ditto for babel), so that doesn't jive with my understanding, and it's a popular enough tool that I concede any argument for using .lhcirc.js and .lhciconfig.json.

* at least, it did initially. it has morphed into run configuration over time, which of course kills my argument. but I did some light research on this and can't find a satisfactory source on this definition. from what I currently know, I consider this morph to be a mistake.

patrickhulce commented 4 years ago

Bold Highlighted Proposal

Let's name this file .lighthouserc. Everything is already nested under the "ci" property in preparation for this exact conversation and other lighthouse settings could slide into here nicely.

Everyone with me? :D

Alternative File Names

If we go with anything named specific to CI I would propose removing the "ci" nesting.

Specific Responses

--rc-file should be --config

I had two concerns with going with just config:

  1. Possible confusion and overloaded meaning with the Lighthouse config. While it's unlikely most of our users will think "Lighthouse config" when they see a --config flag, using this terminology to refer to this concept in Lighthouse CI closes the door on what I hoped could be a unifying configuration file ala .lighthouserc.
  2. It's not the configuration itself, it's the path to a config file. This is something that is more bothersome in other tools that do not differentiate between something that represents the value itself and the path to a file that sets the value, especially when both are configurable via the same mechanisms. In this case we have several useful cases where individual properties of objects can be set from the command-line. Naming it just config when we support CLI objects muddies the water by leaving the door open for someone to try --config.someProperty or whatever while --config-file or --config-path is completely unambiguous.

Counter proposals:

remove ability to customize config path

Why? :) Arguing that the automatic detection of the config is an essential part of an MVP is one thing but saying that one can't name it anything other than what we support is unexpected to me. Also personally, the amount of magic and different behavior in magic between different tools is exhausting. If I have the ability to manually specify paths when things aren't working, I do.

I wonder if there is any purpose to have multiple config files for LHCI?

You might use LHCI in different places. @paulirish specifically called out the idea of server configuration and client configuration living together being confusing, and splitting into an (optional) server configuration and standard client configuration seems reasonable. You're of course free to skip the server CLI and use the node package directly but the conveniences of a CLI wrapper that does this config file parsing/env variable mixing/etc is pretty nice.

allow .js configure via package.json

Sure, backlog :)

nit on rc

Sounds like you already know you're losing this one in the JS community ;)

paulirish commented 4 years ago

flag name

connor already said half of this but....

# rollup
-c, --config <filename>     Use this config file (if argument is used but value
                              is unspecified, defaults to rollup.config.js)

# eslint
  -c, --config path::String      Use configuration from this file or shareable config

# babel  
#   no --config equivalent, relies on default path
  --no-babelrc                         Whether or not to look up .babelrc and .babelignore files

# tsc
# no option for setting the config file, but their config is named tsconfig.json

# terser
  --config-file <file>

# prettier
# assumes some defaults but you can also..
  --config <path>          Path to a Prettier configuration file (.prettierrc, package.json, prettier.config.js).

i really like -c and --config personally.

file name

Also worth noting that eslint has called .eslintrc deprecated for a few years now.

I DO NOT LIKE using .lighthouserc as there's no syntax highlighting or any automatic linting from your editor without configuration or an extension.

All the tools above that support a .xxxrc also support variants with an actual filename extension.

While it's unlikely most of our users will think "Lighthouse config" when they see a --config flag, using this terminology to refer to this concept in Lighthouse CI closes the door on what I hoped could be a unifying configuration file ala .lighthouserc.

I dont totally get what you mean here. Can you reword it?

If the "ci" namespacing prop was paving the way for using a config file that works across LH and CI, then a generic --config flag seems to also pave the way for the same thing, right?? 😕

config..

and since we're talking about defaults & configurability... i'll drop in a relevant perspective from Jason this morning: image

also..

and splitting into an (optional) server configuration and standard client configuration seems reasonable.

going along with this... ciclient and ciserver namespacing props? These names suck, but I like the idea that they're configured separately as they're run separately (99% of the time).

patrickhulce commented 4 years ago

It seems I've been overruled on config having confusion potential. w/e that's what I wrote it as originally so we'll move back.

I DO NOT LIKE using .lighthouserc as there's no syntax highlighting or any automatic linting from your editor without configuration or an extension.

When I proposed .lighthouserc I meant as opposed to other basename options like lhci.config, .lighthouserc.* or whatever is totally fine and preferred IMO. Please don't let this misstatement on my part let y'all throw out the baby with the bath water.

and since we're talking about defaults & configurability... i'll drop in a relevant perspective from Jason this morning:

I'm not sure I understand the point of that tweet in this conversation. As we've already established, autodetect and using a sane default is great, planned, on the backlog, and probably fixed by 4pm today, so there's no point debating that. Is this then trying to say that offering the option of placing a config file somewhere other than your root directory is somehow user-hostile?

going along with this... ciclient and ciserver namespacing props? These names suck, but I like the idea that they're configured separately as they're run separately (99% of the time).

Sure, though ci:client and ci:server?

paulirish commented 4 years ago

I DO NOT LIKE using .lighthouserc as there's no syntax highlighting or any automatic linting from your editor without configuration or an extension.

When I proposed .lighthouserc I meant as opposed to other basename options like lhci.config, .lighthouserc.* or whatever is totally fine and preferred IMO. Please don't let this misstatement on my part let y'all throw out the baby with the bath water.

hahaha you got ALL CAPS REACTION PAUL. Heh I didn't really feel that strongly but I did want to indicate something stronger than "i don't like" and my overworked and addled brain overcompensated. 😳 🤐

ah so perhaps what you were going for was using "rc" in the name to help to differentiate from lighthouse's custom config and plugin config, etc.?

that makes sense. :)

and since we're talking about defaults & configurability... i'll drop in a relevant perspective from Jason this morning:

I'm not sure I understand the point of that tweet in this conversation. As we've already established, autodetect and using a sane default is great, planned, on the backlog, and probably fixed by 4pm today, so there's no point debating that. Is this then trying to say that offering the option of placing a config file somewhere other than your root directory is somehow user-hostile?

eh nah. we're just discussing configs and defaults a lot and i wanted to add in this POV. Basically that well-chosen defaults are extremely powerful. A la parcel vs webpack. Anyway, i'm not suggesting any action in particular here.

we'll keep the "config path flag" for now. seems fine.

going along with this... ciclient and ciserver namespacing props? These names suck, but I like the idea that they're configured separately as they're run separately (99% of the time).

Sure, though ci:client and ci:server?

that's dope.

connorjclark commented 4 years ago

remove ability to customize config path Why? :) Arguing that the automatic detection of the config is an essential part of an MVP is one thing but saying that one can't name it anything other than what we support is unexpected to me. Also personally, the amount of magic and different behavior in magic between different tools is exhausting. If I have the ability to manually specify paths when things aren't working, I do.

My perspective was that by not supporting this option initially, we aren't tied to it forever, in case it turns out to be a burden in the future. I don't feel strongly about it for LHCI. We aren't babel (referring to why I assume they don't support custom config paths due to complexity)