Fix: ConfigArrayFactory was ignoring the resolver option in some places

[onigoetz]

Hi,

I was experimenting with ESLint 8 Beta and wanted to create a pull-request, but saw that there was an issue when using the resolver option on ConfigArrayFactory as it was ignored in some functions.

This PR uses the resolver in the slot instead of ModuleResolver.resolve

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Stéphane Goetz (073f2df)

[nzakas]

Nice catch! Can you add some tests to verify this change?

[onigoetz]

Sure no problem.

I don't know how extensively I have to test this, I went for a simple happy case test for parser, plugin and extends.
Since those are the three places where a custom resolver is used.

A regression in those tests would catch that the custom resolver isn't used anymore. All other tests already cover all other cases from the existing methods.

[nzakas]

LGTM. Thanks! Just need another review before merging.

[mdjermanovic]

I was experimenting with ESLint 8 Beta and wanted to create a pull-request, but saw that there was an issue when using the resolver option on ConfigArrayFactory as it was ignored in some functions.

@onigoetz can you please elaborate more on this, is there something that doesn't work well in ESLint 8 Beta because of this issue?

[mdjermanovic]

I'm not sure why CascadingConfigArrayFactory and ConfigArrayFactory constructors have the resolver option instead of just using internal ModuleResolver.resolve. This is only a utility function that resolves the given name relative to the given path, I don't see a reason to pass in another implementation, and it doesn't seem that ESLint is using this option at all. Instead of maintaining this functionality, I think we could just remove it.

[onigoetz]

Hi @mdjermanovic. to answer your question, yes there is something that doesn't work well with ESLint 8 beta.
It's not a bug as it's something intentional that has already been there in ESLint 7 and before.

It's related to the loading of plugins when they aren't declared as dependencies from the main package.json :

• Support having plugins as dependencies in shareable config eslint#3458

There was a workaround : https://github.com/microsoft/rushstack/tree/master/stack/eslint-patch
but now that there are strict exports, this is no longer possible.

I experimented with the idea of having a custom resolver :
https://github.com/swissquote/crafty/blob/f348d9a943006158837973baa7685da611402a12/packages/crafty-preset-eslint/src/resolver.js

Along with a new resolver option in ESLint itself (that would make use of that option of ConfigArrayFactory, that, as you mentioned isn't used yet) :
https://github.com/swissquote/crafty/blob/f348d9a943006158837973baa7685da611402a12/patches/eslint%2B8.0.0-beta.1.patch

I wanted to land this PR first before starting to open a discussion on ESLint on the possibility to do this but then ... well, life happened :)

Where would you recommend me opening the discussion on ESLint's side ? and who should I involve ?
My case is that for some people (mostly tooling that shares ESLint configurations) it's a necessary feature to be able to override the module resolver (or ideally be able to load module by their absolute path like Babel, Webpack or other tools do)

But since that isn't planned for now, we would need an escape hatch before the future configuration model lands ( eslint/rfcs#9 / eslint/eslint#13481 )

Another possibility : add a special use-at-your-own-risk export like for eslint, to be able to modify the resolution behaviour. However I don't know how feasible that would be since the module is bundled

What do you think of this ?

[nzakas]

@mdjermanovic we have this because I was having trouble resolving some modules from the eslint repo. It turned out to be something else, but I did design these constructors to accept a resolver option. Given that, I think it should work. :)

@onigoetz please keep in mind that this is not an officially supported API. It is intended only for use within ESLint, so it’s not recommended to rely on. Additionally, we will be moving away from using this config system in the future, so you’ll likely need to change what you’re doing when that happens.

[onigoetz]

@nzakas Thanks for the clarifications, yes I totally understand that these APIs are internal and shouldn't be relied upon. I also know that a future configuration system will replace this.

However, the current workaround won't work anymore and the future configuration system isn't ready yet, which creates a gap for all users who would want (need?) this feature (quite a few according to the number of subscriptions in the issues listed in RFC 9) and means all these users will have to skip ESLint 8 for the time being.

[mdjermanovic]

There was a workaround : https://github.com/microsoft/rushstack/tree/master/stack/eslint-patch
but now that there are strict exports, this is no longer possible.

I think this can work with ESLint 8, the objects it needs are available in the main export.

e.g. require("@eslint/eslintrc").Legacy.ConfigArrayFactory

[onigoetz]

Yes, but I'm trying to modify the resolve method on ModuleResolver, which is either hardcoded for some places, or the object itself is using Object.freeze, making it impossible to patch it.
Otherwise, all options to ConfigArrayFactory are stored as slots in a WeakMap and can't be accessed anywhere outside the module (on purpose), so that's not an option either.

[mdjermanovic]

Yes, but I'm trying to modify the resolve method on ModuleResolver, which is either hardcoded for some places, or the object itself is using Object.freeze, making it impossible to patch it.
Otherwise, all options to ConfigArrayFactory are stored as slots in a WeakMap and can't be accessed anywhere outside the module (on purpose), so that's not an option either.

I understand that you have a different use case, my note was about the patch you linked to with concerns that it can't work with ESLint 8 - I think it can work by using a different way to access the objects it wants to patch.

[onigoetz]

Yes, it's possible to retrive or use the ModuleResolver using this method:

require("@eslint/eslintrc").Legacy.ModuleResolver.resolve

But as resolve is an export, the bundled version has an Object.freeze around it.

And since my patch would be require("@eslint/eslintrc").Legacy.ModuleResolver.resolve = someNewResolver that wouldn't fly.

[mdjermanovic]

You're right, we're re-exporting a namespace object and rollup by default freezes it.

Okay, I'm not opposed to this change if it will help (though, I'm not sure how, it seems that it would be more helpful if we would un-freeze the ModuleResolver object).

[mdjermanovic]

Along with a new resolver option in ESLint itself (that would make use of that option of ConfigArrayFactory, that, as you mentioned isn't used yet) :
https://github.com/swissquote/crafty/blob/f348d9a943006158837973baa7685da611402a12/patches/eslint%2B8.0.0-beta.1.patch

eslintrc functionalities are frozen and we're not accepting any options related to it because we're working on the new config system.

If your concerns are that https://github.com/microsoft/rushstack/tree/master/stack/eslint-patch (or its copies) will stop working, we can discuss that. I'm not sure if this fix helps since it's unlikely that we'll add a resolver option to ESLint API and/or ESLint CLI.

[onigoetz]

eslintrc functionalities are frozen and we're not accepting any options related to it because we're working on the new config system.

I understand.

If your concerns are that https://github.com/microsoft/rushstack/tree/master/stack/eslint-patch (or its copies) will stop working, we can discuss that. I'm not sure if this fix helps since it's unlikely that we'll add a resolver option to ESLint API and/or ESLint CLI.

Yes I would like to discuss this, where can we continue this discussions?
And since you know the project much better than I do, what are the possibilities you see to go forward on this?

[mdjermanovic]

@onigoetz by looking at modern-module-resolution.ts and patchModuleResolver.js, I believe the only blocker for supporting ESLint 8 is the fact that ModuleResolver is now a frozen object. If you can confirm that, then I wouldn't be opposed to removing Object.freeze() from the bundle, although we don't support patching. That could be done by setting output.freeze: false in rollup config.

@nzakas would you agree with that? It seems that packages like @rushstack/eslint-patch and its variants used to overcome problems described in eslint/eslint#3458 have a significant number of users.

[onigoetz]

That would help a lot, one small thing to keep in mind though. When generating the bundle the function references are flattened. so if the input is:

import * as ModuleResolver from "./shared/relative-module-resolver.js"

class ConfigArrayFactory {
  _loadParser() {
     ModuleResolver.resolve(...)
  }
}

// ...

const Legacy = { ModuleResolver };

export { Legacy }

Will be bundled as

function resolve() {}

const ModuleResolver = Object.freeze({ resolve });

// ...

class ConfigArrayFactory {
  _loadParser() {
     resolve(...)
  }
}

// ...

const Legacy = { ModuleResolver };

export { Legacy }

Because of tree shaking and other minification, ModuleResolver.resolve() is simplified to resolve(). Which means that it would be possible to patch ModuleResolver but all methods refering to it directly (all except one in the codebase today, without this PR) will still refer directly to the original function even if the ModuleResolver is patched.

By merging this PR, the variable would refer to the full ModuleResolver stored in the slot, which is a reference to the original ModuleResolver and thus would allow patching.

[nzakas]

Sorry, this conversation has gone in too many directions for me to follow. Can we merge this and can we open a separate issue to discuss any remaining concerns?

[mdjermanovic]

By merging this PR, the variable would refer to the full ModuleResolver stored in the slot, which is a reference to the original ModuleResolver and thus would allow patching.

Good catch! I didn't notice that.

[mdjermanovic]

Can we merge this and can we open a separate issue to discuss any remaining concerns?

Agreed, @onigoetz can you please open a new issue to discuss further steps?

[onigoetz]

Sure, should I create it in this repository or in eslint/eslint ?

[mdjermanovic]

Sure, should I create it in this repository or in eslint/eslint ?

Yes, it might be the best place.

[mdjermanovic]

Sure, should I create it in this repository or in eslint/eslint ?

Sorry, I misread the question. I think it would be better to discuss this in eslint/eslint

[onigoetz]

I have to admit I was confused by the answer but was going to create it in eslint/eslint

[mdjermanovic]

LGTM, thanks!