Running with custom module resolver is 3x slower, as resolution is invalidated by TS compiler every time

[rdsedmundo]

• I have tried restarting my IDE and the issue persists.
• I have updated to the latest version of the packages.
• I have read the FAQ and my problem is not listed.

Preface

I'm opening this here more as a discussion because the repo doesn't have the Discussions feature enabled, as this might be a problem to be fixed on the TypeScript compiler itself. See the Additional info for more context.

Repro

Any repo that provides a custom module resolver suffers from this, I tried in different projects and got the same results.

The actual implementation of the resolveModuleNames can be as simple as (moduleNames) => moduleNames.map(n => undefined) and it is still way slower.

Expected Result

Should run with similar speeds.

Actual Result

Runs 3x slower because the resolution is invalidated over and over again.

Additional Info

I have a custom module resolver with cache, and running @typescript-eslint with it presents a 3x slowdown compared to using TS's native resolution. I've debugged it a bit (with my limited knowledge about the TS compiler) and found that the root cause may be that the TS compiler is invalidating all the resolutions whenever a new file is checked because the Program is re-synchronized and re-created no matter what, as a custom module resolution was provided.

Even though my custom resolveModuleName has cache and returns the cached paths immediately after the first seconds of running, the overhead of invalidating all the resolutions and re-creating the Program several times is still way too great and causes this significant slowdown.

Paging a couple of folks that I'd appreciate any input here on whether there's something we can do in the @typescript-eslint side to avoid this, or a suggestion of what could be changed on the TS compiler side to better accommodate custom resolutions and invalidations.

@bradzacher because you were involved with the module resolution issue.
@uniqueiniquity because you added the original custom Program support and may have some insights on this.
@sheetalkamat because you originally introduced the userProvidedResolution check on the TS compiler.

Example run:

Original TS
synchronizeProgram called 1047 times
createNewProgram called 1 times
resolveModuleNames called 1744 times

Custom module resolution
synchronizeProgram called 1047 times
createNewProgram called 1047 times
resolveModuleNames called 1,766,289 times

Custom module resolution with TS compiler patched, forcing the variable userProvidedResolution=false
synchronizeProgram called 1047 times
createNewProgram called 1 times
resolveModuleNames called 1687 times

Workaround

Creating a custom ts.Program by hand and passing it through parserOptions.programs.

Versions

package	version
@typescript-eslint/typescript-estree	4.29.3
TypeScript	4.3.2
node	4.17.0

[rdsedmundo]

@bradzacher do you happen to have any thoughts here? By any chance are you able to send this to someone from the TS team, or would you think that's more appropriate if I re-open this there?

It seems that @sheetalkamat no longer works on TS (judging by her Github activity history) and I'm not sure who else I could mention here that could help.

[bradzacher]

Hey sorry - I don't currently have a direct contact for the TS team. I believe the people we used to work with have left the company or the project.

We need to re-engage with them to rebuild the relationship, but unfortunately that takes time and effort - two things we're in short supply of as volunteer maintainers.
These APIs are also very niche which unfortunately puts them low on the priority list.

[JoshuaKGoldberg]

Following up on this a bit to clear my issue queue: @rdsedmundo we don't have the bandwidth to take a look at this 😞. If you want to start a discussion with the TypeScript folks to get a recommendation or new API for typescript-eslint, that would be great.

[rdsedmundo]

Thanks @JoshuaKGoldberg, I'll try to open a parallel issue on the TypeScript repository to see if they have something to say about it, if they say it's a wontfix then I'd just close this one here too.

[rdsedmundo]

Just opened microsoft/TypeScript#48057, let's see if they can help with anything.

[rdsedmundo]

This is being addressed on the TypeScript side on the issue I linked above, so I believe we can close it from here.

[JoshuaKGoldberg]

Awesome that they're going to investigate this on the TS side!

Let's keep this issue open as we might have to take action once it's resolved, to pull in new features or whatnot.

[rdsedmundo]

@JoshuaKGoldberg TS has finally closed and released a new feature on 4.92.2 that allows providing a custom hasInvalidatedResolutions function in the form (filePath: string) => boolean, which solves the problem I originated reported here, but we need to allow the user to provide it as a parserOptions, as right now we only allow moduleResolver directly.

[bradzacher]

Part of me wonders if there are actually users of this API beyond you, Edmundo.

I'm not sure if us maintaining this integration provides much benefit for the wider community given the increased API surface we need to maintain, as well as the internal TS API surface we need to maintain.

Examples:

• this issue here wherein we need to add additional configuration surface to support performant usage of the feature - which will involve careful design and validation of the new module resolver config and handling of the different versions of the config. Also need to consider different versions of TS that might have different behaviours.
• for the 5.0.0 migration the name of the method has changed to resolveModuleNameLiterals - meaning we'll need to branch inside of our parser to support both old AND new TS versions, which adds complexity and overhead that's really hard for us to test and maintain.

I think that we probably want to remove this entirely from our API in the next major, and those few in the community that actively want this may be better off using patch-package to add whatever additional, advanced features they might need to support their bespoke setup.

Thoughts @JoshuaKGoldberg?

[rdsedmundo]

I understand where you coming from, and as a matter of fact I'm using pnpm patch (patch-package that works properly for pnpm) for the new hasInvalidatedResolutions config as I didn't have time to make a PR here.

I wonder if we could just make a generic option, so instead of allowing individual properties to be passed (customModuleResolver, hasInvalidatedResolutions, now resolveModuleNameLiterals), we could just allow everything that can be attached to a TS.Program in an option in the likes of extra.programOverrides. We don't even need to write tests for their behaviors (as we have now for the custom module resolution, for example), as this would be a responsibility of TS itself, so we'd possibly reduce the maintenance burden. But if you want to drop them fully for the low traction it seems reasonable to me, as I know it's one more thing to maintain.

By the way, thanks for letting me know of the TS 5.0 renaming, I wasn't aware yet.

[JoshuaKGoldberg]

remove this entirely from our API in the next major

Yeah 😕 as much as I'd love to support this edge case, we really don't have the bandwidth to do it justice. Our budget for investigating the TypeScript integrations side of things is going to be dedicated to performance (#6575).

just make a generic option

Interesting idea! I like it at first glance. +1 to putting the burden of logic on users.

@bradzacher?

[bradzacher]

The issue with a generic option is that it exposes the implementation detail that we use a specific TS API internally - which we know is not always true (single run mode) and may not hold true in the future (eg #6172, #6575).

For example - imagine if we did execute on one of those redesigns and create an API compatible redesign of the internals - if we had a generic "pass through" option then we wouldn't be able to ship such a refactor without a breaking change.

With the patch solution users are reaching into our internals to make changes - so it's on them to update their work as we change our internals. With an option we instead have to be careful and deliberate about changing our internals, and it could restrict us.

I'm not sure - personally I think that without some solid user demand I think it's best to just bow out of support. If we were some form of a native package wherein our code was locked away - an option for sure would be great! But patch-package is a solid alternative, for now.

[JoshuaKGoldberg]

@bradzacher I'm confused - your comment seems to indicate support of removing this feature & not adding in a generic option. But adding the accepting prs tag indicates otherwise.

You raise a good point in your message and have persuaded me it's in our best interest not to add the generic option (or at least table the investigation until the performance/rearchitecture investigations all settle). Given that the only consumer on this thread is already using patch-package, I'm in favor now of just removing custom module resolution logic altogether.

[JoshuaKGoldberg]

OH #6609 🤦 I see what you mean by accepting prs. Never mind 😄 same page.