-
Notifications
You must be signed in to change notification settings - Fork 784
Add package deduplication, --deduplicatePackages #2361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 27 commits
9b349c1
dc9841e
d81c18e
358bd71
ed7017c
f98b377
ac7cfc5
43f6e08
b99c0ff
e25f2bc
044eee2
d47b3a7
d7e8232
af6f313
03a3d8f
27d78af
cf0bc18
fb1e84f
614dafd
cae2fba
34155c9
68627e3
2bb7300
b966d60
9d0f1d2
a6956ef
15e0d64
8cbc494
2744db9
bb3e995
31b3a1e
a05a21e
ce80a21
013aa78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,9 +110,10 @@ func (p *Program) GetPackageJsonInfo(pkgJsonPath string) *packagejson.InfoCacheE | |
| return nil | ||
| } | ||
|
|
||
| // GetRedirectTargets implements checker.Program. | ||
| // GetRedirectTargets returns the list of file paths that redirect to the given path. | ||
| // These are files from the same package (same name@version) installed in different locations. | ||
| func (p *Program) GetRedirectTargets(path tspath.Path) []string { | ||
| return nil // !!! TODO: project references support | ||
| return p.redirectTargetsMap[path] | ||
| } | ||
|
|
||
| // gets the original file that was included in program | ||
|
|
@@ -241,6 +242,14 @@ func (p *Program) UpdateProgram(changedFilePath tspath.Path, newHost CompilerHos | |
| if !canReplaceFileInProgram(oldFile, newFile) { | ||
| return NewProgram(newOpts), false | ||
| } | ||
| // If this file is part of a package redirect group (same package installed in multiple | ||
| // node_modules locations), we need to rebuild the program because the redirect targets | ||
| // might need recalculation. A file is in a redirect group if it's either a canonical | ||
| // file that others redirect to, or if it redirects to another file. | ||
| if _, ok := p.deduplicatedPathMap[changedFilePath]; ok { | ||
|
||
| // File is either a canonical file or a redirect target; either way, need full rebuild | ||
| return NewProgram(newOpts), false | ||
jakebailey marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| // TODO: reverify compiler options when config has changed? | ||
| result := &Program{ | ||
| opts: newOpts, | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| package fourslash_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/microsoft/typescript-go/internal/fourslash" | ||
| "github.com/microsoft/typescript-go/internal/testutil" | ||
| ) | ||
|
|
||
| func TestDuplicatePackageServices_fileChanges(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| defer testutil.RecoverAndFail(t, "Panic on fourslash test") | ||
| const content = `// @noImplicitReferences: true | ||
| // @Filename: /node_modules/a/index.d.ts | ||
| import X from "x"; | ||
| export function a(x: X): void; | ||
| // @Filename: /node_modules/a/node_modules/x/index.d.ts | ||
| export default class /*defAX*/X { | ||
| private x: number; | ||
| } | ||
| // @Filename: /node_modules/a/node_modules/x/package.json | ||
| { "name": "x", "version": "1.2./*aVersionPatch*/3" } | ||
| // @Filename: /node_modules/b/index.d.ts | ||
| import X from "x"; | ||
| export const b: X; | ||
| // @Filename: /node_modules/b/node_modules/x/index.d.ts | ||
| export default class /*defBX*/X { | ||
| private x: number; | ||
| } | ||
| // @Filename: /node_modules/b/node_modules/x/package.json | ||
| { "name": "x", "version": "1.2./*bVersionPatch*/3" } | ||
| // @Filename: /src/a.ts | ||
| import { a } from "a"; | ||
| import { b } from "b"; | ||
| a(/*error*/b);` | ||
| f, done := fourslash.NewFourslash(t, nil /*capabilities*/, content) | ||
| defer done() | ||
|
|
||
| f.GoToFile(t, "/src/a.ts") | ||
| f.VerifyNumberOfErrorsInCurrentFile(t, 0) | ||
|
|
||
| testChangeAndChangeBack := func(versionPatch string, def string) { | ||
| // Insert "4" after the version patch marker, changing version from 1.2.3 to 1.2.43 | ||
| f.GoToMarker(t, versionPatch) | ||
| f.Insert(t, "4") | ||
|
|
||
| // Insert a space after the definition marker to trigger a recheck | ||
| f.GoToMarker(t, def) | ||
| f.Insert(t, " ") | ||
|
|
||
| // No longer have identical packageId, so we get errors. | ||
| f.VerifyErrorExistsAfterMarker(t, "error") | ||
|
|
||
| // Undo the changes | ||
| f.GoToMarker(t, versionPatch) | ||
| f.DeleteAtCaret(t, 1) | ||
| f.GoToMarker(t, def) | ||
| f.DeleteAtCaret(t, 1) | ||
|
|
||
| // Back to being identical. | ||
| f.GoToFile(t, "/src/a.ts") | ||
| f.VerifyNumberOfErrorsInCurrentFile(t, 0) | ||
| } | ||
|
|
||
| testChangeAndChangeBack("aVersionPatch", "defAX") | ||
| testChangeAndChangeBack("bVersionPatch", "defBX") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| /src/a.ts(5,3): error TS2345: Argument of type 'import("/node_modules/c/node_modules/x/index").default' is not assignable to parameter of type 'import("/node_modules/a/node_modules/x/index").default'. | ||
| Types have separate declarations of a private property 'x'. | ||
|
|
||
|
|
||
| ==== /src/a.ts (1 errors) ==== | ||
| import { a } from "a"; | ||
| import { b } from "b"; | ||
| import { c } from "c"; | ||
| a(b); // Works | ||
| a(c); // Error, these are from different versions of the library. | ||
| ~ | ||
| !!! error TS2345: Argument of type 'import("/node_modules/c/node_modules/x/index").default' is not assignable to parameter of type 'import("/node_modules/a/node_modules/x/index").default'. | ||
| !!! error TS2345: Types have separate declarations of a private property 'x'. | ||
|
|
||
| ==== /node_modules/a/index.d.ts (0 errors) ==== | ||
| import X from "x"; | ||
| export function a(x: X): void; | ||
|
|
||
| ==== /node_modules/a/node_modules/x/index.d.ts (0 errors) ==== | ||
| export default class X { | ||
| private x: number; | ||
| } | ||
|
|
||
| ==== /node_modules/a/node_modules/x/package.json (0 errors) ==== | ||
| { "name": "x", "version": "1.2.3" } | ||
|
|
||
| ==== /node_modules/b/index.d.ts (0 errors) ==== | ||
| import X from "x"; | ||
| export const b: X; | ||
|
|
||
| ==== /node_modules/b/node_modules/x/index.d.ts (0 errors) ==== | ||
| content not parsed | ||
|
|
||
| ==== /node_modules/b/node_modules/x/package.json (0 errors) ==== | ||
| { "name": "x", "version": "1.2.3" } | ||
|
|
||
| ==== /node_modules/c/index.d.ts (0 errors) ==== | ||
| import X from "x"; | ||
| export const c: X; | ||
|
|
||
| ==== /node_modules/c/node_modules/x/index.d.ts (0 errors) ==== | ||
| export default class X { | ||
| private x: number; | ||
| } | ||
|
|
||
| ==== /node_modules/c/node_modules/x/package.json (0 errors) ==== | ||
| { "name": "x", "version": "1.2.4" } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly like that this loop is like this, versus having the package ID before collect and then deduping with it, but there are some weird ordering constraints here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stared at this for a while and figured out how to actually make it behave the way I wanted without this loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, this loop was working around the fact that packages get their IDs from incoming edges, so there was a race. Now it's way less weird