-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
- Start Date: 2019-11-04 | ||
- RFC PR: https://github.com/eslint/rfcs/pull/46 | ||
- Authors: Pete Gonzalez ([@octogonz](https://github.com/octogonz)) | ||
|
||
# Add resolveRelativeToConfigFile setting | ||
|
||
## 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. | ||
|
||
## Motivation | ||
|
||
For the more narrow (and perhaps more common) scenario tackled by this RFC, please refer to | ||
[this comment](https://github.com/eslint/eslint/issues/3458#issuecomment-516666620) from ESLint issue 3458. | ||
|
||
The more general set of requirements is already spelled out in RFC #14. | ||
|
||
## Detailed Design | ||
|
||
Change [line 841 of config-array-factory.js]( | ||
https://github.com/eslint/eslint/blob/586855060afb3201f4752be8820dc85703b523a6/lib/cli-engine/config-array-factory.js#L845) | ||
from this: | ||
|
||
```js | ||
try { | ||
filePath = ModuleResolver.resolve(request, relativeTo); | ||
} catch (resolveError) { | ||
``` | ||
|
||
...to this: | ||
|
||
```ts | ||
try { | ||
filePath = ModuleResolver.resolve(request, importerPath); | ||
} catch (resolveError) { | ||
``` | ||
|
||
Gate this change behind a new option `resolveRelativeToConfigFile` in `.eslintrc.js`. It is an optional boolean | ||
value that is `false` by default. Thus this behavior will be off by default. | ||
|
||
A complete implementation is already provided in [ESLint PR 12460](https://github.com/eslint/eslint/pull/12460). | ||
|
||
## Documentation | ||
|
||
The PR will include documentation for the new option. | ||
|
||
## Drawbacks | ||
|
||
The `resolveRelativeToConfigFile` feature does not consider all possible design considerations, such as | ||
conflicts between plugins. The PR also assumes that `resolveRelativeToConfigFile` is not set back to `false` | ||
after it has been set to `true`. Once it is enabled, it affects all subsequent module resolutions. | ||
|
||
This is acceptable because it's a temporary workaround. It's not meant to be an ideal design. | ||
|
||
## Backwards Compatibility Analysis | ||
|
||
No impact, because the option is off by default. | ||
|
||
## Alternatives | ||
|
||
If RFC #14 can be completed and implemented within a reasonable timeframe, then this workaround would not be needed. | ||
|
||
## Open Questions | ||
|
||
Since a PR has already been created, please provide feedback on the implementation details. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. The place to discuss design is here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 design != implementation, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
## Help Needed | ||
|
||
None. | ||
|
||
## Frequently Asked Questions | ||
|
||
### In order to modify one line of logic, do we really need to wait an entire month for the RFC process? | ||
|
||
Yes, the ESLint maintainers have requested this. | ||
|
||
### Why should we accept a solution with known limitations? | ||
|
||
Back in July, the workaround was presented as a | ||
[monkey patch](https://github.com/eslint/eslint/issues/3458#issuecomment-516716165). | ||
Several people are using it in large multi-project repos for serious shipping applications, | ||
and they've reported that it works correctly. Thus, it's "good enough" for many people who would be otherwise blocked. | ||
|
||
### Why can't everyone just use a monkey patch then? | ||
|
||
Monkey patching is awkward and brittle. An .eslintrc.js file should not probe into the ESLint binary | ||
and overwrite its module objects at runtime. It may be acceptable for small projects, but at a large company, | ||
project admins would be rightly concerned about the supportability of such a solution. | ||
|
||
## Related Discussions | ||
|
||
None. |
There was a problem hiding this comment.
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.