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

Change Request: Allow to patch ModuleResolver in ESLint 8

Closed
1 task done
onigoetz opened this issue Sep 8, 2021 · 19 comments · Fixed by #15146
Closed
1 task done

Change Request: Allow to patch ModuleResolver in ESLint 8 #15036

onigoetz opened this issue Sep 8, 2021 · 19 comments · Fixed by #15146
Labels
accepted archived due to age core enhancement
Projects

Comments

@onigoetz
Copy link

onigoetz commented Sep 8, 2021

ESLint version

8.0.0-beta.1

What problem do you want to solve?

In the current version of ESLint, it's currently not possible to have plugins as dependencies in shared configs (as detailed in #3458)
It is however possible to patch ModuleResolver to make this work :

The patch isn't anything fancy:
https://github.com/swissquote/crafty/blob/master/packages/crafty-preset-eslint/src/patchModuleResolver.js#L78-L95

But it works better than --resolve-plugins-relative-to since it allows to have logic for more than one directory and doesn't require the end-user to add it to each of his calls to eslint

Now since this patching accesses the modules directly at their location inside the eslint or @eslint/eslintrc package it won't be possible anymore as

  • they will be using exports in package.json
  • @eslint/eslintrc is bundled so we can't refer to a module directly
  • Rollup automatically wraps exports in Object.freeze thus effectively blocking require("@eslint/eslintrc").Legacy.ModuleResolver.resolve = someNewResolver
  • Even if patching worked, Rollup simplifies module references to directly refer to the resolve function so it wouldn't take the patch into consideration anyway. ( Fix: ConfigArrayFactory was ignoring the resolver option in some places eslintrc#53 (comment) )

What do you think is the correct solution?

The easiest solution I would see and was proposed by @mdjermanovic : disable the freezing of objects by Rollup : eslint/eslintrc#53 (comment)

That would need to be combined with something that makes sure to not refer to the function directly but to refer to the ModuleResolver object so that the patching works.

This summary is a follow up of the discussion in eslint/eslintrc#53

Participation

  • I am willing to submit a pull request for this change.
@onigoetz onigoetz added core enhancement triage labels Sep 8, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Sep 8, 2021
@mdjermanovic mdjermanovic moved this from Needs Triage to Feedback Needed in Triage Sep 8, 2021
@mdjermanovic mdjermanovic removed the triage label Sep 8, 2021
@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 8, 2021

I support this. We generally don't support or encourage patching, but I wouldn't mind this small change on @eslint/eslintrc's Legacy API that would allow those packages to support ESLint 8 until we implement the new flat config system where all this will become unnecessary.

@nzakas
Copy link
Member

nzakas commented Sep 15, 2021

If all that’s needed is to change the Rollup config, I’m fine with that.

That would need to be combined with something that makes sure to not refer to the function directly but to refer to the ModuleResolver object so that the patching works

What is “something” here? A lint rule? A convention?

@onigoetz
Copy link
Author

onigoetz commented Sep 15, 2021

What is “something” here? A lint rule? A convention?

I didn't have anything specific in mind, I just know that Rollup is smart and if you do an import * as ModuleResolver from "./shared/relative-module-resolver.js" and then use ModuleResolver.resolve(), rollup simplifies it to simply resolve() which won't be patcheable.

I can make a PR for the Rollup config, and check that second part as well

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 15, 2021

We could add tests that replace ModuleResolver.resolve with some function, call APIs that are using ModuleResolver.resolve(), and then assert that the replacement function was called. That way, we would know if some change in the code breaks this.

@nzakas
Copy link
Member

nzakas commented Sep 16, 2021

This all feels a bit too hacky for my taste. Bending over backwards to support monkey patching doesn’t seem like the right solution here. I’d like us to take a step back and think this through a bit more.

My understanding is that you are monkey patching ModuleResolver.resolve so that when ESLint uses it, it’s using the patched version rather than the default, is that correct? And you’re using ESLint via the JS API and not from the CLI?

If that’s the case, adding a resolve option to the ESLint class constructor feels like the most appropriate approach to support this use case. It would go away with the new config system, but so will a bunch of other options we currently have.

@onigoetz
Copy link
Author

onigoetz commented Sep 16, 2021

I agree that it's a bit hackish and having a proper solution would be much easier to handle.

Yes your understanding is correct.
Although I'm using ESLint both by the JS API (through webpack, rollup and gulp) and the CLI (for when users want to run eslint --fix )

Adding a resolve option to the ESLint class constructor would already fix 80% of my use case, that would be great.

@nzakas
Copy link
Member

nzakas commented Sep 16, 2021

Is it feasible to use the ESLint constructor in place of your CLI usage right now?

There's definitely not a clean solution for the CLI use case as far as I can tell.

@onigoetz
Copy link
Author

onigoetz commented Sep 17, 2021

Not perfect, but I can live with it

I agree, I don't see a clean way to do it in the CLI either, other than passing an absolute path to a JS module but only automated tools would be able to use it correctly

@nzakas
Copy link
Member

nzakas commented Sep 29, 2021

@eslint/eslint-tsc do we feel comfortable adding a resolve option to the ESLint class for this?

@btmills
Copy link
Member

btmills commented Sep 29, 2021

Since it’d be temporary, and it’s less hacky than alternative solutions, I’m okay with it.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 29, 2021

I have concerns that this won't work for the other mentioned patch @rushstack/eslint-patch. That patch replaces resolver only for ConfigArrayFactory#_loadPlugin, it needs ctx.filePath, and its usage is to run the patch from user's .eslintrc.js so there is no an app in the middle that could pass resolver to ESLint constructor.

So we might end up with adding a new option on the public API and un-freezing ModuleResolver, in which case it seems easiest for everyone to just un-freeze ModuleResolver now.

The easiest solution I would see and was proposed by @mdjermanovic : disable the freezing of objects by Rollup : eslint/eslintrc#53 (comment)

That would need to be combined with something that makes sure to not refer to the function directly but to refer to the ModuleResolver object so that the patching works.

An alternative that wouldn't require output.freeze: false, and wouldn't depend on rollup's internal logic is to do export default { resolve } instead of export { resolve } in relative-module-resolver.js. That would make ModuleResolver.resolve being replaceable when the package is used as ESM, so rollup shouldn't do any freezing or flattening in the bundle.

@nzakas
Copy link
Member

nzakas commented Sep 29, 2021

That would make ModuleResolver.resolve being replaceable when the package is used as ESM

I’m just not onboard with the monkey patching approach. This feels very, very fragile, and the folks who are already monkey patching should know that. Perpetuating that seems like a bad idea.

I don’t think our goal here should be to support every hack that’s currently out there, but rather, come up with an approach that we can live with. If adding a new option solves most but not all of the problems, I think that’s where we should start.

And anything we do to address this is temporary and likely will be obsolete in the next year, so I don’t think we should spend too much brainpower here.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 30, 2021

We don't support hacks but we're adding to our main ESLint API an option whose only purpose is to hack into ESLint's internal logic, and the option can solve only a small subset of problems that would be solved by unlocking one property on our legacy API. This doesn't make sense to me, but I won't block it.

@mdjermanovic mdjermanovic added the accepted label Sep 30, 2021
@nzakas
Copy link
Member

nzakas commented Oct 1, 2021

This doesn't make sense to me, but I won't block it.

I don’t find this type of comment productive. We shouldn’t be thinking in terms of blocking anything. We should be thinking in terms of how to satisfy use cases in a way that we all can live with.

We don't support hacks but we're adding to our main ESLint API an option whose only purpose is to hack into ESLint's internal logic

I don’t understand this argument. All of the options to the ESLint class change internal logic.

Fundamentally, we are dealing with a messy situation where packages are modifying APIs that they shouldn’t be modifying to force ESLint to behave in a way it was never intended to behave. My main concern with unfreezing the API is this:

That would need to be combined with something that makes sure to not refer to the function directly but to refer to the ModuleResolver object so that the patching works.

This feels like a constraint that is difficult to guarantee. However, given that the folks who are doing this are in a “void your warranty” situation, I’d be open to dropping this constraint in favor of letting the monkey-patchers submit PRs to fix anything that might break. I don’t want to be making guarantees that this approach will never break, but if they are willing to shoulder some of the maintenance cost, I’m open to unfreezing the API.

@mdjermanovic
Copy link
Member

mdjermanovic commented Oct 1, 2021

We don't support hacks but we're adding to our main ESLint API an option whose only purpose is to hack into ESLint's internal logic

I don’t understand this argument. All of the options to the ESLint class change internal logic.

I'll try to better explain my view on this option. The interface is resolve(moduleName, relativeToPath). The function doesn't have any context provided, it doesn't know what kind of entity is being resolved and why it should be resolved relative to relativeToPath. This should be only a utility function that resolves modules per its two parameters, while the logic about how different kinds of entities in different contexts are resolved is in ESLint (e.g., configs relative to the file where they are referenced in "extends", plugins relative to project's config file or relative to --resolve-plugins-relative-to, etc.). Custom resolvers make sense when the built-in utility cannot work per its specification in some environments. That's not the case here, this option would be used to pass in a function that does not resolve modules relative to relativeToPath, and thus alters the logic that isn't its responsibility. This is also fragile because the function isn't aware of the context, so it may be called in a context where its alternative resolution isn't desirable.

@btmills
Copy link
Member

btmills commented Oct 3, 2021

I'm sympathetic to the argument that the resolve option by itself only addresses one of these cases. What if _loadPlugin() also passed ctx.filePath to resolve()? ModuleResolver.resolve() wouldn't need it, but that would remove the need to monkey patch _loadPlugin().

@nzakas
Copy link
Member

nzakas commented Oct 5, 2021

@mdjermanovic thanks for explaining. I guess my point is that all of the options are fragile in some way, and I find explicit approaches to be favorable to implicit approaches in that case.

@btmills I really don’t want to be changing existing internal APIs related to eslintrc at this point. The whole thing is a house of cards as it is.

My main goal at this point is to find something that works until we can get to the new config system. If the path of least resistance right now is to unfreeze the resolve method, then let’s do that. I just want to be clear that this is in no way an officially supported feature and if it breaks in some other way in the future, we aren’t going to spend time fixing it. Fair?

@btmills
Copy link
Member

btmills commented Oct 5, 2021

As I understand it, un-freezing ModuleResolver would not solve the other half of this that overrides _loadPlugin(). Would un-freezing ModuleResolver also un-freeze ConfigArrayFactory to allow patching that too?

https://github.com/microsoft/rushstack/blob/8b2252a00fd57eb4f440bd8f2d6ba61d2ab369cd/stack/eslint-patch/src/modern-module-resolution.ts#L121

@nzakas
Copy link
Member

nzakas commented Oct 6, 2021

No, but I’m not sure that’s needed. I don’t believe ConfigArrayFactory.prototype is frozen.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted archived due to age core enhancement
Projects
Triage
Complete
Development

Successfully merging a pull request may close this issue.

4 participants