Add "package deduplication", sorta #2361
Draft
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1080 (sorta??)
The old compiler did "package deduplication" through source file redirection. This allowed two duplicate packages in different parts of
node_modules(but with the same name / package.json version) to be "compatible" by way of them being the same ASTs even though they are at different paths (and so at runtime, probably are not compatible).Doing the same thing in the new codebase is maybe impossible?
The old compiler did this by essentially proxying the original
SourceFileviaObject.create(sourceFile)(the original file is the prototype 😨) plus overriding itsid/symbol. I don't see how we could emulate that in Go without makingSourceFileinto an interface.Even if we could do that, file loading is now concurrent. In the original compiler, whichever package came first won. But there are no winners in concurrent loading, and by not picking a winner early, we can't really "undo" any additional file loads caused by files we don't use.
My personal preferred option is to do nothing about this. I think it's objectively correct to complain when you attempt to use a symbol from one lib and give it to another. They will be different symbols!
But, this still seems to come up. And yes, it sucks to have nominal classes that are seemingly incompatible even though they will really behave the same.
This PR attempts an alternative solution.
We still load everything. But, we pick a winner after loading and record the dupes. Then, when the checker needs to cross-compare symbols, we see if the symbols are actually just dupes, and say they are compatible.
This is yucky, so there's now a
--disablePackageDeduplicationoption (defaulting tofalse, bleh) to control this behavior. Some people who have tried the new compiler have actually liked that we errored in these cases...I am unsure if this is really the right solution. I feel like what'll happen is that something like the language service won't be aware of this (since symbol equality won't apply), or there's some part of the checker that still has to change. And, who knows what little bugs I have. Our tests are very sparse on this.
But, I wanted to start thinking about this problem, at least.