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

New: Add resolveRelativeToConfigFile setting #46

Closed
wants to merge 4 commits into from

Conversation

octogonz
Copy link

@octogonz octogonz commented Nov 5, 2019

Summary

RFC #14 proposes a way for a shared ESLint config package to supply its own plugin dependencies, rather than imposing that responsibility on every consumer of the package. But because that RFC seeks to design a comprehensive solution, its discussion has been open for a long time without encouraging progress. In the interim, RFC 46 proposes a temporary workaround. It will allow users to opt-in to a resolution behavior that works well in practice (despite having some known limitations). This this new resolveRelativeToConfigFile option would be designated as experimental, with the intent to remove it when/if the ideal solution finally ships.

Related Issues

@jsf-clabot
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@ljharb
Copy link

ljharb commented Nov 5, 2019

This would not be sufficient for the airbnb config, because users would have to be told to add it as a top-level config. Any solution - temporary or otherwise - must be doable without further action from the user.

@octogonz
Copy link
Author

octogonz commented Nov 5, 2019

@ljharb If AirBNB is the first config that gets loaded that needs this feature, then the flag will probably get set in time. I can test that and follow up.

However given that this is an "experimental" feature, I would prefer to instruct users to copy+paste the option into their top-level eslinrc.js. It is only one extra config line, so that doesn't seem to be a cumbersome requirement, and this makes it explicit that they are using a special resolution mode. For example if one of their dependencies is incompatible, they might not realize that AirBNB had "opted in" behind the scenes. Does that make sense?

@ljharb
Copy link

ljharb commented Nov 5, 2019

An experimental feature isn’t one that shared configs can risk making a requirement.

@yordis
Copy link

yordis commented Nov 5, 2019

I had been using require.resolve for everything, just let NodeJS resolve things as most NodeJS people will expect things to be resolved.

ModuleResolver.resolve = (moduleName: string) => {
  return require.resolve(moduleName);
};

So far, no issues with AirBnB package

@siric
Copy link

siric commented Nov 5, 2019

users would have to be told to add it as a top-level config. Any solution - temporary or otherwise - must be doable without further action from the user.

That is an extremely strange restriction you're imposing here. An opt-in setting should require no action from the user? That defeats the whole point of opting in.

An experimental feature isn’t one that shared configs can risk making a requirement.

Shared configs should not make this a requirement, the decision should be left to the end-user whether to opt-in to this feature or not. Users can choose to either set the resolveRelativeToConfigFileoption to true or continue to install peer deps as per default. For the people willing to opt-in, they can be made aware of the feature's experimental state through proper documentation.

@ljharb
Copy link

ljharb commented Nov 5, 2019

@siric the whole problem that needs solving is that users need to do extra stuff. The solution will only be something that removes that burden - ie, something that the shared config can opt into on its own.

@octogonz
Copy link
Author

octogonz commented Nov 5, 2019

I feel like there's a difference between copy+pasting resolveRelativeToConfigFile: true one time -- versus copy+pasting approximately 7 separate package.json dependencies, plus assuming responsibility for ensuring that their version numbers are always up-to-date and compatible with whatever the shared config was tested with.

That said, @ljharb lemme see if we can support your approach.

@octogonz
Copy link
Author

octogonz commented Nov 6, 2019

This would not be sufficient for the airbnb config, because users would have to be told to add it as a top-level config. Any solution - temporary or otherwise - must be doable without further action from the user.

I tested this, and it actually works fine to put resolveRelativeToConfigFile: true inside the shared config. 🎉

I had misremembered the details a bit: ESLint visits extended config files one at a time, collecting options and resolving referenced plugins/configs. It doesn't merge/evaluate the options until the very end. Thus if resolveRelativeToConfigFile was a normal option, we wouldn't know its final state until after all the module resolution had occurred, which is too late. To avoid this, my PR stores the resolveRelativeToConfigFile in the context and sets it to true as soon as resolveRelativeToConfigFile: true is first encountered. To reduce confusion due to ordering, we don't allow it to be reset back to false -- once it's true, it's true from then on. This seems reasonable enough. But there is still an edge case where the behavior will be different for any resolutions that occur before resolveRelativeToConfigFile: true is first encountered. For example, it might matter whether you order airbnb to appear before/after some other plugin in the top-level config.

None of this matters for my intended usage, where resolveRelativeToConfigFile: true is set in the top-level config. Seems like it actually works reasonably well for your scenario as well. But if you can think of some better way to evaluate the option, we could consider that.

@ljharb BTW if you're seriously interested in this feature, could you test the PR branch with your own use case? I'm most interested in hearing about any real-world (versus speculative) situations where the behavior is confusing or incorrect. Thanks!

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Hi. Thank you for your contribution! I really appreciate your work to advance this long-standing issue. But please mind that this problem is not simple and we have more work to land your proposal.

But because that RFC seeks to design a comprehensive solution, its discussion has been open for a long time without encouraging progress.

Because nobody is active to solve that issue currently, rather than seeking a comprehensive solution.

I have a plan to retry my old proposal then solve the problem in the future (after I have finish eslint/eslint#12274, #39, #40, #42, and #45). However, I'm a volunteer and busy in my job, so if you get active to solve this problem, it's really great.


Now, as you know, the plugin loading is the base of the ESLint core, then the change of the plugin loading has many side-effects. And some CLI options affect the plugin loading. This RFC has to describe how your design addresses the side-effects and collaborating with other options.

The most important of my request is "don't be broken silently!" At least, when ESLint encountered unexpected configuration/structure, ESLint must print what happened to users. This is important even if it's "experimental" because users should be able to distinguish what is expected behaviors and what is not.

Let's discuss this.

● What does it happen if two shareable configs depended on different versions of the same plugin?

The reason that ESLint loads plugins from the current working directory is the identification of rules.

plugins:
- foo
rules:
  foo/a-rule: error

In the above, ESLint has to identify foo/a-rule to exactly one rule. This is an essential requirement. However, in your proposal, shareable configs can have own version of plugins. What does it happen if two shareable configs depended on different versions of the same plugin? (please mind "don't be broken silently!")

● What does it happen if config files that are in subdirectories have resolveRelativeToConfigFile option?

ESLint merges multiple config files in ancestor directories. For example, if the following file structure existed,

<root>
├ src
│ └ foo.js
├ test
│ ├ foo.js
│ └ .eslintrc.yml
└ .eslintrc.yml

ESLint merges test/.eslintrc.yml and ./.eslintrc.yml when it lints test/foo.js. At the time, ESLint reuses the parsed result of ./.eslintrc.yml with the linting of src/foo.js. Is there a mix of two plugin loading strategy? (please mind "don't be broken silently!")

● What does it happen if plugin configs have resolveRelativeToConfigFile option?

Plugin configs can have shared configs.

extends: "plugin:foo/recommended"

If the plugin config has the resolveRelativeToConfigFile option, what does it happen? Is there a mix of two plugin loading strategy? (please mind "don't be broken silently!")

● What does it happen if --resolve-plugins-relative-to option along with resolveRelativeToConfigFile option?

If --resolve-plugins-relative-to is present and a config has the resolveRelativeToConfigFile option, what does it happen? (please mind "don't be broken silently!")


On the whole, I feel that the option should not be in config files. Similar to --resolve-plugins-relative-to CLI option, it should be a CLI option. And it should throw fatal errors on plugin conflictions.


## Alternatives

If RFC #14 can be completed and implemented within a reasonable timeframe, then this workaround would not be needed.
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see more details of the "Alternative" section.

I have described four alternatives and there are multiple proposals around this: #5, #9, a part of #7, #14, a derivation of #14.

Please describe the pros and cons of this proposal that are relative to those.


## Open Questions

Since a PR has already been created, please provide feedback on the implementation details.
Copy link
Member

Choose a reason for hiding this comment

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

No. The place to discuss design is here.

Copy link
Author

@octogonz octogonz Nov 8, 2019

Choose a reason for hiding this comment

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

🤔 design != implementation, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. It's not clear how the implementation change affects user-facing behavior. Even if the change is one line, it may be many side-effects. The purpose of RFCs is to clarify how the proposed changes ESLint from the user's perspective. Then the "Open Questions" section describes the open questions about design decisions.

@mysticatea mysticatea added Initial Commenting This RFC is in the initial feedback stage and removed triage labels Nov 6, 2019
@octogonz
Copy link
Author

octogonz commented Nov 8, 2019

@mysticatea Thanks for your feedback! The answer to all those what-if questions is "Nobody wants to do that. It's not supported with this workaround. We should report an error."

But let me think about the individual questions and see how easy it is to implement error reporting.

However, I'm a volunteer and busy in my job, so if you get active to solve this problem, it's really great.

I might consider this, but let's first focus on getting people unblocked ASAP. The full solution will probably be a breaking change, so if it gets implemented it should wait and ship with a major release of ESLint.

On the whole, I feel that the option should not be in config files. Similar to --resolve-plugins-relative-to CLI option, it should be a CLI option.

@mysticatea In my view ESLint has a very problematic reliance on CLI options. This topic probably warrants its own RFC, but let me try to summarize it here, since you brought it up.

Consider these options which (as far as I understand) can only be specified using CLI parameters:

  • --ext [String] Specify JavaScript file extensions - default: .js
  • --resolve-plugins-relative-to path::String A folder where plugins should be resolved from, CWD by default
  • The files to be linted, for example eslint "lib/**"

If you want to lint all the files in your project, you can't do it without knowing this information.
These settings rarely change for a given project. They belong in eslintrc.js.

Now, what happens when I open VS Code in the project folder? The ESLint add-in reads eslintrc.js and starts trying to underline lint errors in my editor.

But how is VS Code supposed to find the --resolve-plugins-relative-to setting, which is buried in some build script somewhere that's different for each project. Their answer is to copy your CLI options into a proprietary project-specific settings file for VS Code. Well what if some other people at your company use WebStorm? Probably to copy+paste into some other proprietary format.

All this awkwardness would go away if ESLint allowed important config information to be stored in the config file, rather than insisting on passing it via CLI.

@octogonz
Copy link
Author

octogonz commented Nov 8, 2019

  • The files to be linted, for example eslint "lib/**"

(The .eslintignore config file can be thought of as a way to specify this, if you can figure out how to negate all the ignore wildcards. However when I tried this approach, it seemed that eslint "**" would crawl every single file and inefficiently test it against .eslintignore. That file matcher didn't seem to understand how to skip an entire directory.)

@mysticatea
Copy link
Member

"Nobody wants to do that. It's not supported with this workaround. We should report an error."

OK. Therefore, would you update RFC with the decision? Please keep mind to describe user-facing behaviors rather than implementation.

I feel the current "Detailed Design" section describes implementation rather than specification. The implementation is worthful to confirm feasibility, but it should be an appendix. The goal of RFCs is to clarify what user-facing changes are.

I think we need the following stuff.

  • This proposal adds resolveRelativeToConfigFile option that a boolean value into config files.
  • What does it happen if resolveRelativeToConfigFile:true is present?
  • What does it happen if resolveRelativeToConfigFile:false is present?
  • What does it happen if resolveRelativeToConfigFile is present in subdirectories?
  • What does it happen if resolveRelativeToConfigFile is present in shareable configs?
  • What does it happen if resolveRelativeToConfigFile is present in plugin configs?
  • What does it happen if resolveRelativeToConfigFile along with --resolve-plugins-relative-to is present?

I might consider this, but let's first focus on getting people unblocked ASAP. The full solution will probably be a breaking change, so if it gets implemented it should wait and ship with a major release of ESLint.

I don't think the current implementation has enough quality for public beta. I think that the goal of the public beta is to find potential problems. However, the current spec/implementation has many problems clearly and it's too early to advance.

Also, this proposal doesn't look to need a major release.


In my view ESLint has a very problematic reliance on CLI options. This topic probably warrants its own RFC, but let me try to summarize it here, since you brought it up.

I know. Therefore, we have been reducing the settings that config files cannot configure. For example, #20 lets us configure additional kinds of files (similar --ext) in config files. #22 lets us configure --no-inline-config, --report-unused-disable-directives, and --ignore-pattern/.eslintignore in config files. A part of #13 will let us configure --rulesdir and will solve eslint/eslint#3458.

However, there are some options that we cannot set in config files. One is the setting that changes how ESLint loads config files. We cannot change the loading strategy of config files by the config files because we must decide the loading strategy before loading config files. The --resolve-plugins-relative-to is one of them. I think that resolveRelativeToConfigFile is one of them as well. In fact, your PoC is really problematic. It makes a mix of two loading strategy and the loaded config is broken silently.

@octogonz
Copy link
Author

octogonz commented Nov 8, 2019

However, there are some options that we cannot set in config files. One is the setting that changes how ESLint loads config files. We cannot change the loading strategy of config files by the config files because we must decide the loading strategy before loading config files.

This is stated as a requirement, but it's not. It's just a technical hitch that we need to hack around (until the temporary setting is no longer needed).

The requirement is: "ESLint users need to opt-in to a different loading strategy, and it must be specified in eslintrc." Otherwise, editors can't discover the setting easily (far as I can tell), which is not the experience we want.

In fact, your PoC is really problematic. It makes a mix of two loading strategy and the loaded config is broken silently.

Can anyone step forward and say: "Here is a real world project where I tried to use resolveRelativeToConfigFile , and it's not working for me because of mixed loading strategies."

So far the objections seem to be mostly speculations.

@yordis
Copy link

yordis commented Nov 8, 2019

I would love to have the ability to control the loader.

For us, there is no magic, it had been module.resolve under the hood and let NodeJS with CommonJS to figure out how to load plugins, configs, or whathaveyou.

@octogonz
Copy link
Author

octogonz commented Nov 8, 2019

What if we drop @ljharb's requirement about the airbnb package being able to opt-in?

We could say that resolveRelativeToConfigFile is ONLY allowed in the first .eslintrc.js file to get loaded. It cannot appear in any other place.

You gave this example:

<root>
├ src
│ └ foo.js
├ test
│ ├ foo.js
│ └ .eslintrc.yml
└ .eslintrc.yml

Is it always easy to identify the root-level ./eslintrc.yml file? Is it always the first config to get loaded?

If we impose this constraint, it would eliminate all the concerns about mixed loading strategies.

To further harden things, we could also provide a way for a package like airbnb to assert that the caller has opted-in. For example, maybe airbnb sets a config option like:

   requireResolveRelativeToConfigFile: true,

....and then ESLint would report an error if someone tries to load airbnb without first setting resolveRelativeToConfigFile=true in their eslintrc.js.

Then your design questions are really easy to answer:

  • What does it happen if resolveRelativeToConfigFile is present in subdirectories?

Error.

  • What does it happen if resolveRelativeToConfigFile is present in shareable configs?

Error.

  • What does it happen if resolveRelativeToConfigFile is present in plugin configs?

Error.

  • What does it happen if resolveRelativeToConfigFile along with --resolve-plugins-relative-to is present?

Error.

● What does it happen if two shareable configs depended on different versions of the same plugin?

Error.

@octogonz
Copy link
Author

octogonz commented Nov 8, 2019

For us, there is no magic, it had been module.resolve under the hood and let NodeJS with CommonJS to figure out how to load plugins, configs, or whathaveyou.

Unlike TSLint, ESLint doesn't trust plugin authors to avoid naming collisions.

For example, if eslint-plugin-example defines a rule my-rule, then ESLint calculates its name as example/my-rule.

This means you can't simply use NodeJS require() without ESLint's involvement. For example, if eslint-config-my-shared calls require(eslint-plugin-example), then the rule object would appear to be in that package, and ESLint would calculate the rule name as my-shared/example.

This is solvable, but not without a major redesign of the config file loader. Frankly the world would be much simpler if ESLint reported an error when two plugins try to use the same name, rather than inventing a complex disambiguation that produces very long rule names.

@yordis
Copy link

yordis commented Nov 9, 2019

Unlike TSLint, ESLint doesn't trust plugin authors to avoid naming collisions.

And we are fine with it, we decide to opt-in for those issues as long as it means we remove 7+ dependencies per package, the maintains burden is too much as the organization grows.

It is better for us to rely on our organization config to figure out these edge-cases in one place.

@mysticatea
Copy link
Member

mysticatea commented Nov 9, 2019

This is stated as a requirement, but it's not. It's just a technical hitch that we need to hack around (until the temporary setting is no longer needed).

That's a requirement. Or, it's a technical constraint. If we ignore this constraint, ESLint will enter infinite looping in some situations or just go broken. It will be called a spec bug.


Can anyone step forward and say: "Here is a real world project where I tried to use resolveRelativeToConfigFile , and it's not working for me because of mixed loading strategies."
So far the objections seem to be mostly speculations.

It's a fortune. They had not encountered the spec bug.


We could say that resolveRelativeToConfigFile is ONLY allowed in the first .eslintrc.js file to get loaded. It cannot appear in any other place.

It sounds like our good common ground. But we need another mechanism to realize it. Because we cannot determine the first .eslintrc.js file until shareable configs and plugin configs were loaded.

Is it always easy to identify the root-level ./eslintrc.yml file? Is it always the first config to get loaded?

No. In ./test directory, ESLint loads ./test/.eslintrc.yml at first and doesn't load ./.eslintrc.yml if the ./test/.eslintrc.yml has the root:true setting. And it's not determined whether the ./test/.eslintrc.yml has the root:true setting or not until ESLint loads all shareable configs and plugin configs. Because it's applied to the extender if shareable configs or plugin configs have root:true.

This is the reason that #22 didn't bring some CLI options to config files. If we want to determine the top-level config file, we have to add the setting that disables config files of subdirectories. And I thought that the setting should be discussed in another RFC than #22 to land #22 ASAP.


As you may have noticed, changing the loading strategy is not a simple way. It conflicts with many other parts of ESLint. Even if we solved the problem of the loading strategy mix, there is one more matter.

Instead, would you consider another simple way? I think that either localPlugin setting or Plugin renaming is better and easier. That will solve several matters along with eslint/eslint#3458: (1) bringing the functionality of --rulesdir CLI option to config files, (2) being able to use free names for plugins.

@octogonz
Copy link
Author

octogonz commented Nov 9, 2019

@mysticatea Ok cool, lemme take some time this weekend to read all those issues and think about it. These are some good insights.

(BTW looks like you are in Japan. I will be in Tokyo later this month. If it's not far away possibly we could meet up and chat about the broader design. 🙂)

@mysticatea
Copy link
Member

(BTW looks like you are in Japan. I will be in Tokyo later this month. If it's not far away possibly we could meet up and chat about the broader design. 🙂)

Yes, I live in Japan. I can go to Tokyo on a weekend. But... I cannot listen to English in most. My English ability is poor that I cannot act on GitHub without Google Translate. 😅

@octogonz
Copy link
Author

I'm closing this RFC and the associated PR. We ultimately decided to publish our workaround as an NPM package, rather than trying to integrate it into ESLint. You can find it here: @rushstack/eslint-patch Thanks all!

@octogonz octogonz closed this Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Initial Commenting This RFC is in the initial feedback stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants