Change Request: Allow to patch ModuleResolver in ESLint 8

[onigoetz]

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 :

• https://github.com/microsoft/rushstack/tree/master/stack/eslint-patch
• https://github.com/swissquote/crafty/blob/master/packages/crafty-preset-eslint/src/patchModuleResolver.js

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.

[mdjermanovic]

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]

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]

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]

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]

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]

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]

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]

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]

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

[btmills]

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

[mdjermanovic]

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]

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]

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.

[nzakas]

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]

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]

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]

@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]

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]

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