Enabling enhanced-resolve Causes Errors

[manuth]

Expected Behaviour

Running webpack using ts-loader with enhanced-resolve enabled causes errors as described in #1504 and #1505.

Actual Behaviour

webpack should still run properly with ts-loader.

Steps to Reproduce the Problem

1. Install ts-loader@9.4.0
2. Install spica
3. Import setTimeout from spica/global
4. Try to compile the module using webpack with ts-loader
5. Take note that the compilation fails

Location of a Minimal Repository that Demonstrates the Issue.

• https://github.com/falsandtru/pjax-api/tree/76ad3769e23497a15817914a92787089c5dfa8cc
• https://github.com/falsandtru/clientchannel/tree/ec42cd2d3df396c6ee9f5cc82465cd6898174fda

Thanks a lot to @falsandtru for providing the demonstrations!

This issue is related to #1506, #1504 and #1505
The error has been introduced in #1503

[manuth]

Sadly, I could only boil this down to 2 resolve-processes which had different results, so we might have a tough time working out what we have to change.

Resolve-Calls (using ts-loader v9.4.0)

Resolve Base	enhanced-resolve result	typescript result	actual result	desired result
./node_modules/spica/global.ts	./global.type.ts	undefined	"./node_modules/spica/global.type.ts"	undefined
.	./index.ts	"./index.d.ts"	"./index.ts"	"./index.d.ts"

The desired result is taken from taken from ts-loader@9.3.

Case 1:

• spica/global.ts has been imported.

• spica/global.ts, then again, imports ./global.type.ts.

• typescript only resolves module specifiers which have their output extension (such as .js) or no extension.

• enhanced-resolve doesn't care about the file extension being .ts and finds the global.type.ts file

Case 2:

This is taken from https://github.com/falsandtru/pjax-api/tree/76ad3769e23497a15817914a92787089c5dfa8cc

• The specifier . points to the directory containing the package.json file.

• typescript looks for the types referenced in the package.json file's types or exports field and thus finds the index.d.ts file

• enhanced-resolve can't find the main file becuause the package is not compiled. Thus, it searches for files with the configured extensions and finds the index.ts file

Questions

• Is there ever a situation where enhanced-resolve should be prioritized?
  • alias setting
• How do we know enhanced-resolves result should be prioritized higher than typescripts? As it looks, enhanced-resolve tends to return incorrect, undesired results.

[johnnyreilly]

Is there ever a situation where enhanced-resolve should be prioritized?

As you say, alias. Apart from that perhaps not. ts-loader aims to be a drop in replacement for TSC so it's mostly that behaviour we care about.

How do we know enhanced-resolves result should be prioritized higher than typescripts? As it looks, enhanced-resolve tends to return incorrect, undesired results.

Maybe the answer is we're only interested if a typescript lookup has failed?

[manuth]

So my idea based on this is the following:

• Create 2 instances of ResolveSync: One with the alias, one without the alias setting
• If the results are different => return result
• Otherwise return undefined

Because of the result being undefined, ts-loader will automatically use typescripts result.

That way we can be sure all results are returned for the sake of the alias setting.
Technically, this should lower the number of errors in the aliasResolution tests back again.

[johnnyreilly]

Sounds like an interesting idea. I'm wondering what effect this might have on performance - hopefully not too significant. Feels like it could work.

Question: if the enhanced-resolve has been missing in action for all this time, what have been the issues that has caused? Are there use cases we haven't been supporting as a consequence?

Essentially, what changes if we do this? What do we gain?

[manuth]

Not sure, tbh - I thought ehanced-resolve might be here for reasons.
The console output of the comparison-tests reported less errors - no idea whether this means different output in general.

[manuth]

I just gave it a go and it does work.
However, I just read through this comment suggesting that the use of enhanced-resolve should be dropped completely.
Especially for the performance gain, this might be a very good idea.

Sadly, replacing it with this.getResolve does not seem to work as this method works with Promises while ts-loader is completely written with sync code.

Do you think we should completely drop it?
It was never actually running anyways.

[manuth]

Or we could just leave it as-is which will make it easier to implement enhanced-resolve or this.getResolve at a later point in time.

[johnnyreilly]

At the moment I'm inclined to leave as is. The surprising thing is that a "broken" enhanced-resolve doesn't seem to be an issue. It doesn't seem to be impacting users.

I am curious as to the impact on the comparison tests of the new approach. If you have the bandwidth, I'd be curious to see what that looks like. But no worries if not

[manuth]

Currently, enhanced-resolve is disabled as per #1506

I will quickly set up a PR containing a (part-wise) re-enabled enhanced-resolve there, we should be able to see the differences in the comparison-tests.

[manuth]

The changes which happen when enabling enhanced-resolve (or a small number of cases), a few comparison-tests would change.

You might want to have a look at it: #1509

[johnnyreilly]

That's the direction I'm thinking as well