setting environments in shared config?

[rhbecker]

Hey, I'm still getting up to speed on eslint and your implementation of shared config, so apologies if any of this feedback isn't quite right. And I'm happy to generate PRs for anything you think work addressing ...

env configured in 3 files ...

1. legacy.js

   a. 'browser': true
   b. 'node': true
   c. 'amd': false
   d. 'mocha': false
   e. 'jasmine': false

2. rules/node.js: 'node': true

3. rules/es6.js: 'es6': false

issues

1. Why is legacy.js disabling amd, mocha, and jasmine? Aren't environments off until explicitly enabled?
2. Why is legacy.js enabling browser and node? While these may be the most common usage contexts, that seems to overstep the role of this style-guide-based shared config, which might be used in a variety of circumstances. Even if that weren't the case, any "efficiency" gained in enabling both by default would seemingly be lost when most users then have to disable one or the other. Maybe I'm missing something?
3. Similarly - and this may seem like an odd question - why is rules/node.js enabling node? The fact that legacy.js extends rules/node.js, and the fact that everything else extends legacy.js, means that node is always enabled. If the rules in rules/node.js are "safe" to be included everywhere, I think it makes sense to keep that rule set in the chain of extensions, but then I don't think it makes sense for the env to be configured in there. If the rules in rules/node.js are NOT "safe" to be included everywhere, its place in the extension chain needs to be altered, at which point, it might end up making sense to leave the env config in there, similar to how es6 env config makes sense as you have it, in a rule set only included in an extension chain used by those intending to use es6.
4. Depending on how you respond to the prior 2 issues, this might be moot, but it seems redundant that node env is enabled in both legacy.js and rules/node.js.

[ljharb]

1. to ensure that the state of the world is known. if you want them to be true, you'd turn them on after extending the config.
2. both browser and node, so that restrictions on both apply. it's not about "efficiency", it's about "best default".
3. if someone requires the node rules by themselves, they won't have it already turned on. Each file can be required by itself. I'm not sure why es6 is "false" in the es6 config - @taion, you added this in daea8a6 / Eslint config defaults #526, any idea why?
4. It's not redundant, because someone might require one, or the other, or both, and all three should have node set to "true".

[taion]

@ljharb I copied it blindly from https://github.com/walmartlabs/eslint-config-defaults/blob/a5b8466a48bf3251e060ce96d3d7cdc7edd6e9e6/rules/eslint/es6/airbnb.js#L5.

[ljharb]

ah, per http://eslint.org/docs/user-guide/configuring#specifying-environments looks like "es6: true" would turn on a bunch of stuff, so "false" lets us explicitly turn on features.

[rhbecker]

Please feel free to ignore me, but since I've bothered to investigate, I figure I might as well continue to share.

responses

to ensure that the state of the world is known. if you want them to be true, you'd turn them on after extending the config.

Eslint offers 22 environments, so it seems somewhat arbitrary that these 3 are specified as false, and somewhat arbitrary that legacy.js is the only place it's done. To truly know the state of the world, a user is gonna ultimately have to read through or search the hierarchy being extended. I'd much rather do either task on the smallest amount of config declaration possible to get the job done.

both browser and node, so that restrictions on both apply

I suspect one of us (probably me) is misunderstanding what env is for. The docs (here and here) imply that enabling an env suppresses lint errors of the no-undef variety for a set of predefined global variables specific to that environment.

So, I don't understand what you mean by "restrictions on both apply". If I'm writing a browser app, why would I want node globals, etc.? Might that not mask issues in my code if I'm wrongly referring to some global that doesn't actually exist in the environment in which it runs?

if someone requires the node rules by themselves, they won't have it already turned on

I guess I see that as a pro, rather than a con. If I'm extending a config called rules/node.js, I'd expect it to contain rules, and not env or other settings.

It's not redundant, because someone might require one, or the other, or both

I don't necessarily want to focus on this one, given my wish to have rules/ configs contain only rules, but as is, if I require legacy.js and that extends rules/node.js, and rules/node.js has already enabled node, then what value is added by repeating the enabling in legacy.js?

some ideas

I'm still learning, so please take these with a grain of salt, but based on my current understanding of things ...

1. env is only ever set to false if a known true of an extended config needs overriding. That way, when I search the hierarchy I'm extending for 'env', I'll get a very small handful of matches that will make it immediately apparent what the "state of the world" is.
2. A case could be made that, other than es6, you not even touch env at all, leaving it to those extending your shared config to specify that. This is assuming your intention is to provide an eslint representation of what's specified in the style guide.
3. If you do need to set env, you might only do so in the configs (index, base, legacy) currently located in the root directory, and not in configs within the rules/ directory, which should only contain rules.
4. If you do need to set env, you might only do so in configs specific to the usage scenario for which the config exists. e.g. If you want to define a config for use by node apps, create node-es5 and node-es6 configs, and enable the node env in there, and similarly for browser, and any other env usages you wish to pre-define (though, again, I'm not sure I see the point for this lib).

[ljharb]

Keep in mind that these were written at a time with eslint did default some envs (and rules) to on, so that's probably the real reason they're set to false.

Since we now require a newer version of eslint, I'm fine with removing the "false"s but I really don't see the issue.

Do you have an actual issue caused by the env values being set? I don't think it's necessarily helpful to make changes solely for ideological reasons.

[rhbecker]

Obviously, it can get pretty subjective, but I don't believe the issues I'm raising are purely ideological. In my experience, the more superfluous code / config you litter a code base with, the harder it becomes to maintain, and the harder it is for new users to understand. There's also risk that some of that litter ends up conflicting with new additions, etc.

As I said, I'm happy to do the work, but I don't want to bother unless you're on board. And I also need you to vet whether the stuff I'm saying even makes sense. I'm still new to eslint and your repo.

[rhbecker]

I'm gonna call it a night. Thanks for the responses and feel free to reflect for a while. None of this is a rush.

[ljharb]

Every top-level app should be defining its own environment settings. The legacy endpoint is legacy, and I'm not concerned with its philosophical correctness one way or the other.

Thanks for the discussion!