Skip to content

Conversation

@jakebailey
Copy link
Member

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 SourceFile via Object.create(sourceFile) (the original file is the prototype 😨) plus overriding its id/symbol. I don't see how we could emulate that in Go without making SourceFile into 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 --disablePackageDeduplication option (defaulting to false, 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.

Comment on lines +7 to +8
-// === /node_modules/a/node_modules/x/index.d.ts ===
+// === /node_modules/b/node_modules/x/index.d.ts ===
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff exists because go-to-def is not aware of the deduping, instead opting to go to the "actual" file rather than the deduped winner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deduplicate doubly-installed packages

2 participants