From 97eba351d68131d212474e03e14eda5b71105c4d Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Wed, 9 Oct 2024 17:43:52 -0700 Subject: [PATCH] fix(cli): reduce tsconfig paths precedence below pnpm workspace project deps (#7060) An issue found at a client when upgrading from an ancient cli version to more recent. This changes the order of how dependencies can be resolved, reducing the precedence of tsconfig paths to be one of the lowest priorities instead of being on-par with relative file imports. The client use case and the test case added is when a tsconfig file has `paths` essentially just for the IDE dev experience or maybe for legacy non-bazel (or non-pnpm-workspace) reasons. Imports of local workspace projects should use `:node_modules/@wksp/project-a` via package.json instead of `//projects/a` vis tsconfig.json. --- ### Changes are visible to end-users: yes - Searched for relevant documentation and updated as needed: yes - Breaking change (forces users to change their own code or config): no - Suggested release notes appear below: yes Pnpm workspace project dependencies resolved with `aspect configure` are now higher priority then resolving dependencies via tsconfig paths. ### Test plan - Covered by existing test cases - New test cases added GitOrigin-RevId: 7cf0f95489f5600001351d36a7f13500bc465b62 --- gazelle/js/generate.go | 36 +++---- gazelle/js/resolve.go | 94 ++++++++++++------- gazelle/js/target.go | 3 - gazelle/js/tests/pnpm_workspace/tsconfig.json | 10 ++ 4 files changed, 84 insertions(+), 59 deletions(-) create mode 100644 gazelle/js/tests/pnpm_workspace/tsconfig.json diff --git a/gazelle/js/generate.go b/gazelle/js/generate.go index 93598d165..f557a954e 100644 --- a/gazelle/js/generate.go +++ b/gazelle/js/generate.go @@ -589,7 +589,6 @@ func (ts *typeScriptLang) collectProtoImports(cfg *JsGazelleConfig, args languag Lang: LanguageName, Imp: dts, }, - Alt: []string{}, ImportPath: imp, SourcePath: sourceFile, }) @@ -653,30 +652,25 @@ func (ts *typeScriptLang) collectImports(cfg *JsGazelleConfig, rootDir, sourcePa } for _, importPath := range parseResults.Imports { + if cfg.IsImportIgnored(importPath) { + BazelLog.Tracef("Import ignored: %q", importPath) + continue + } + // The path from the root workspacePath := toImportSpecPath(sourcePath, importPath) - if !cfg.IsImportIgnored(importPath) { - alternates := make([]string, 0) - for _, alt := range ts.tsconfig.ExpandPaths(sourcePath, importPath) { - alternates = append(alternates, toImportSpecPath(sourcePath, alt)) - } - - // Record all imports. Maybe local, maybe data, maybe in other BUILD etc. - result.Imports = append(result.Imports, ImportStatement{ - ImportSpec: resolve.ImportSpec{ - Lang: LanguageName, - Imp: workspacePath, - }, - Alt: alternates, - ImportPath: importPath, - SourcePath: sourcePath, - }) + // Record all imports. Maybe local, maybe data, maybe in other BUILD etc. + result.Imports = append(result.Imports, ImportStatement{ + ImportSpec: resolve.ImportSpec{ + Lang: LanguageName, + Imp: workspacePath, + }, + ImportPath: importPath, + SourcePath: sourcePath, + }) - BazelLog.Tracef("Import: %q -> %q (alt: %v)", workspacePath, importPath, alternates) - } else { - BazelLog.Tracef("Import ignored: %q -> %q", workspacePath, importPath) - } + BazelLog.Tracef("Import: %q -> %q (via %q)", sourcePath, workspacePath, importPath) } return result diff --git a/gazelle/js/resolve.go b/gazelle/js/resolve.go index 645fc0804..047c89f43 100644 --- a/gazelle/js/resolve.go +++ b/gazelle/js/resolve.go @@ -366,41 +366,9 @@ func (ts *typeScriptLang) resolveImport( return Resolution_Override, res, nil } - possible := make([]resolve.ImportSpec, 0, 1) - possible = append(possible, impStm.ImportSpec) - for _, expandedImp := range impStm.Alt { - possible = append(possible, resolve.ImportSpec{Lang: impStm.Lang, Imp: expandedImp}) - } - - // Gazelle rule index. Try each potential expanded path - for _, eImp := range possible { - if matches := ix.FindRulesByImportWithConfig(c, eImp, LanguageName); len(matches) > 0 { - filteredMatches := make([]label.Label, 0, len(matches)) - for _, match := range matches { - // Prevent from adding itself as a dependency. - if !match.IsSelfImport(from) { - filteredMatches = append(filteredMatches, match.Label) - } - } - - // Too many results, don't know which is correct - if len(filteredMatches) > 1 { - return Resolution_Error, nil, fmt.Errorf( - "Import %q from %q resolved to multiple targets (%s) - this must be fixed using the \"gazelle:resolve\" directive", - impStm.ImportPath, impStm.SourcePath, targetListFromResults(matches)) - } - - // The matches were self imports, no dependency is needed - if len(filteredMatches) == 0 { - return Resolution_None, nil, nil - } - - match := filteredMatches[0] - - BazelLog.Tracef("resolve %q import %q as %q", from, impStm.Imp, match) - - return Resolution_Override, &match, nil - } + // Gazelle rule index + if resolution, match, err := ts.resolveImportFromIndex(c, ix, from, impStm); resolution != Resolution_NotFound { + return resolution, match, err } // References to a label such as a file or file-generating rule @@ -413,6 +381,24 @@ func (ts *typeScriptLang) resolveImport( return Resolution_Package, pkg, nil } + // References via tsconfig mappings (paths, baseUrl, rootDirs etc.) + if tsconfigPaths := ts.tsconfig.ExpandPaths(impStm.SourcePath, impStm.ImportPath); len(tsconfigPaths) > 0 { + for _, p := range tsconfigPaths { + pImp := ImportStatement{ + ImportSpec: resolve.ImportSpec{ + Lang: impStm.ImportSpec.Lang, + Imp: toImportSpecPath(impStm.SourcePath, p), + }, + SourcePath: impStm.SourcePath, + ImportPath: impStm.ImportPath, + Optional: impStm.Optional, + } + if resolution, match, err := ts.resolveImportFromIndex(c, ix, from, pImp); resolution != Resolution_NotFound { + return resolution, match, err + } + } + } + // Native node imports if node.IsNodeImport(imp.Imp) { return Resolution_NativeNode, nil, nil @@ -421,6 +407,44 @@ func (ts *typeScriptLang) resolveImport( return Resolution_NotFound, nil, nil } +func (ts *typeScriptLang) resolveImportFromIndex( + c *config.Config, + ix *resolve.RuleIndex, + from label.Label, + impStm ImportStatement) (ResolutionType, *label.Label, error) { + + matches := ix.FindRulesByImportWithConfig(c, impStm.ImportSpec, LanguageName) + if len(matches) == 0 { + return Resolution_NotFound, nil, nil + } + + filteredMatches := make([]label.Label, 0, len(matches)) + for _, match := range matches { + // Prevent from adding itself as a dependency. + if !match.IsSelfImport(from) { + filteredMatches = append(filteredMatches, match.Label) + } + } + + // Too many results, don't know which is correct + if len(filteredMatches) > 1 { + return Resolution_Error, nil, fmt.Errorf( + "Import %q from %q resolved to multiple targets (%s) - this must be fixed using the \"gazelle:resolve\" directive", + impStm.ImportPath, impStm.SourcePath, targetListFromResults(matches)) + } + + // The matches were self imports, no dependency is needed + if len(filteredMatches) == 0 { + return Resolution_None, nil, nil + } + + match := filteredMatches[0] + + BazelLog.Tracef("resolve %q import %q as %q", from, impStm.Imp, match) + + return Resolution_Override, &match, nil +} + func (ts *typeScriptLang) resolvePackageImport(from label.Label, imp string) *label.Label { impPkg, _ := node.ParseImportPath(imp) diff --git a/gazelle/js/target.go b/gazelle/js/target.go index cf768c542..119b8ccf7 100644 --- a/gazelle/js/target.go +++ b/gazelle/js/target.go @@ -13,9 +13,6 @@ import ( type ImportStatement struct { resolve.ImportSpec - // Alternative paths this statement may resolve to - Alt []string - // The path of the file containing the import SourcePath string diff --git a/gazelle/js/tests/pnpm_workspace/tsconfig.json b/gazelle/js/tests/pnpm_workspace/tsconfig.json new file mode 100644 index 000000000..22c9424e8 --- /dev/null +++ b/gazelle/js/tests/pnpm_workspace/tsconfig.json @@ -0,0 +1,10 @@ +// This tsconfig also maps paths but is only for IDEs! +{ + "compilerOptions": { + "paths": { + // The BUILD dependencies should prioritize the `workspace:*` packages + // instead of mapping to the `ts_project` targets that this tsconfig maps to. + "@lib/*": ["lib/*"] + } + } +}