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: Changing the Default Value of --resolve-plugins-relative-to #39

Closed
wants to merge 4 commits into from

Conversation

mysticatea
Copy link
Member

Summary

This RFC changes the default value of --resolve-plugins-relative-to CLI option to work on more varied environments.

Related Issues

@mysticatea mysticatea added the Initial Commenting This RFC is in the initial feedback stage label Sep 28, 2019
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM

@zbrogz
Copy link

zbrogz commented Oct 4, 2019

Can this be merged?

@kaicataldo
Copy link
Member

@zbrogz Please read the documentation here about our RFC approval process.

@mysticatea mysticatea added breaking change This RFC contains breaking changes Final Commenting This RFC is in the final week of commenting and removed Initial Commenting This RFC is in the initial feedback stage labels Oct 14, 2019
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

I've identified one small typo (provided a suggestion to hopefully make the fix trivial).

Two questions:

  1. Just to confirm: For the case where someone is using --config without --resolve-plugins-relative-to but has plugins installed in their project, they can fix by simply adding something like --resolve-plugins-relative-to ., right?
  2. As an alternative, would it make more sense to always try resolving packages from the location of config files first, and only fall back to --config and --resolve-plugins-relative-to if they are not found attached to the config files? (I'm thinking of monorepo cases where someone could break the linting workflow by trying to use --resolve-plugins-relative-to . without realizing that some of the plugins are in sub-packages. My understanding of this RFC is that --resolve-plugins-relative-to completely stops any other plugin searching, and I wonder if the UX would be better if we implicitly always searched for plugins that are installed next to config files.

…EADME.md

Co-Authored-By: Kevin Partington <platinum.azure@kernelpanicstudios.com>
@mysticatea
Copy link
Member Author

Just to confirm: For the case where someone is using --config without --resolve-plugins-relative-to but has plugins installed in their project, they can fix by simply adding something like --resolve-plugins-relative-to ., right?

Yes, you are right.

As an alternative, would it make more sense to always try resolving packages from the location of config files first, and only fall back to --config and --resolve-plugins-relative-to if they are not found attached to the config files?

It sounds a good idea. I didn't think to load plugins from multiple bases. I will consider it.

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Current RFC text looks good to me, but please let me know what you think about my alternative proposed plugin load strategy (once you have thought about it some more).

@mysticatea mysticatea added Initial Commenting This RFC is in the initial feedback stage and removed Final Commenting This RFC is in the final week of commenting labels Nov 6, 2019
@mysticatea
Copy link
Member Author

I rollbacked this RFC from the final commenting because I have a plan to update this RFC largely.

This RFC has a problem in CLIEngine#getRules() and formatter's rulesMeta property because when ESLint verifies multiple files, it loses the guarantee of the identification of plugin rules.

# Conflicts:
#	designs/2019-changing-default-of-resolve-plugins-relative-to/README.md
@mysticatea
Copy link
Member Author

I have separated this RFC to #39 (this) and #47.

Comment on lines +47 to +51
### About personal config file (`~/.eslintrc`)

This RFC doesn't change the plugin base path for the personal config file (`~/.eslintrc`) because the functionality has been deprecated in [RFC 32](https://github.com/eslint/rfcs/pull/32). ESLint still loads plugins from the current working directory if there is no project config file, even if there is the personal config file.

If people want to use the personal config file, use `--config` option (e.g., `eslint . --config ~/.eslintrc.json`). It should work intuitively.
Copy link

Choose a reason for hiding this comment

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

This feels so unproductive. I'm basically saying: This is poor wording, and you keep telling me how the software works.

### About personal config file (`~/.eslintrc`)

This RFC is essentially unrelated to the handling of personal config files. In particular, we do 
not detect if the user provides `--config ~/.eslintrc` and handle it any different. Therefore, with 
this in effect, we would set `resolvePluginsRelativeTo` to the HOME dir in such cases. This can 
confuse users which might expect we handle a personal config file differently i.e. by instead 
resolving globally installed packages. It could also encourage users to install into the HOME dir. 
However, we clearly do not recommend users install anything into `~/node_modules`. 

Something like that. So I basically change "intuitively" to "confuse".

Does that help?


## Drawbacks

- The new logic is complex a bit. It increases maintenance cost.
Copy link

Choose a reason for hiding this comment

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

Add

  • Given a personal config file, user might feel encouraged to install packages into the HOME dir. See discussion above.

Probably remove "The new logic is complex a bit" because it is not.


If people want to use the personal config file, use `--config` option (e.g., `eslint . --config ~/.eslintrc.json`). It should work intuitively.

### Special handling of `~/`
Copy link

Choose a reason for hiding this comment

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

Also, just to make it clear (and, yes, I know that I'm coming from nowhere here, and leave comments in a repo I may not belong to). IMO (and as a Windows user) a) this is YAGNI and probably has more maintenance cost as the core change, b) this is unrelated and should get its own RFC.

@mysticatea
Copy link
Member Author

I'm closing this RFC because I re-thought that we should not change the default value of --resolve-plugins-relative-to option by --config option while I refresh #47.

Thank you for many feedbacks. Because #47 is the successor of this RFC, please track that!

@mysticatea mysticatea closed this Nov 19, 2019
@mysticatea mysticatea deleted the improve-resolve-plugins-relative-to branch November 19, 2019 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This RFC contains breaking changes 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