fix(linter): flag banned external imports reached through internal projects - #36654
fix(linter): flag banned external imports reached through internal projects#36654leosvelperez wants to merge 8 commits into
Conversation
✅ Deploy Preview for nx-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for nx-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
View your CI Pipeline Execution ↗ for commit 601de4a
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
AgentEnder
left a comment
AgentEnder
left a comment
There was a problem hiding this comment.
Confirmed the fix does what it says. I ran your new specs against master's version of the two source files, and exactly the four tests that assert a nested violation fail there. All 118 pass at HEAD. The feature was fully inert before this.
One thing I'd want changed before it lands.
The violation message never names the package that violated
nestedBannedExternalImportsViolation interpolates imp, which on this path is the internal import. The transitive package that actually matched the constraint never appears.
That was harmless while the branch was unreachable. Now that allowedExternalImports reaches it, the error blames an import that is itself permitted:
A project tagged with "api" is not allowed to import "@mycompany/impl". Nested import found at implName
Same finding, second half: hasBannedDependencies returns one tuple per dependency edge and the rule reports one error per tuple. With your own fixture, implName depending on npm-package (static), npm-awesome-package (static) and npm-package (dynamic) produces 3 byte-identical reports for a single import. findTransitiveExternalDependencies doesn't dedupe, despite its jsdoc promising unique deps.
Adding target.data.packageName to the report data and the message template covers the first half. Keying the filtered tuples by ${dep.source}|${dep.target} covers the second.
Maintainer calls
Three decisions worth naming rather than inheriting. All three read as deliberate to me and I'd land them as they are:
- The gate widening to
allowedExternalImportsgoes past #36519. It's the coherent choice, since the allow branch already ran nested for mixed constraints and fixing only the ban list would leave{allowed: [...]}silently unenforced. But anyone already runningcheckNestedExternalImportsnext to an allow list goes from zero nested errors to one per transitive package outside that list. allowedExternalImports: []now bans every transitive external, because the gate tests truthiness. That matches the direct path.- Nested matching is package-granular, so
['lodash/fp']won't catch a nestedlodash. The options table doesn't mention the asymmetry.
Smaller things
- Every new test has the banned package exactly one hop away. Two hops works, and names the right child, but nothing pins it.
runtime-lint-utils.spec.ts:349is now identical in inputs and expectation to the test at :314.impmeans "import specifier" on the direct path and "package name" on the nested one. A rename would carry that without a comment.
…ojects With checkNestedExternalImports enabled, importing an internal project whose transitive dependencies include a package listed in bannedExternalImports never produced a violation. The subpath-matching guard added to isConstraintBanningProject for deep import bans compares the original import specifier against the external package name, and on the nested path that specifier names the imported internal project, so the guard never matched and the nested check was dead. Match transitive external dependencies against the external package's own name instead of the original import specifier, restoring the behavior the nested check had before the guard was introduced. The direct import path keeps the specifier-based subpath matching.
The nested external-import check only ran for constraints with a bannedExternalImports list. That gate predates allowedExternalImports, so an allowed-only constraint never checked the transitive external dependencies of imported projects, while a direct import of a package outside the list was reported. The gate now accepts an allowed list too, including an empty one, which bans every external package.
…elf-Healing CI Rerun]
The nested external-import violation only showed the internal import and the project owning the nested dependency. Under an allowed-only constraint the blamed import is itself permitted, and the package that matched the constraint never appeared in the error. The report now includes the matched package name in the message.
hasBannedDependencies returned one tuple per dependency edge, so an internal project reaching the same package through several edges (static and dynamic, or several resolved versions of the same package) produced byte-identical errors for a single import statement. findTransitiveExternalDependencies now keeps one edge per project and package, matching the uniqueness its jsdoc already promised.
Dropping the imp parameter from hasBannedDependencies left two tests with identical inputs and expectations, so one is removed. The isConstraintBanningProject parameter is renamed to importSpecifier since it receives the bare package name on the nested path. The checkNestedExternalImports docs now state that nested matching is package-granular.
|
Thanks for the thorough pass @AgentEnder, and for running the specs against master to confirm the fix. Good catch on the message. It now names the package: For the duplicates I went with deduping in Also added a two-hop test, removed the lint-utils test that had become identical to its sibling, renamed the The three maintainer calls stay as you read them: all deliberate. |
… [Self-Healing CI Rerun]
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… [Self-Healing CI Rerun]
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
🎓 Learn more about Self-Healing CI on nx.dev
2 participants
Current Behavior
With
checkNestedExternalImports: true, importing an internal project never reports a violation for that project's transitive external dependencies:bannedExternalImportsconstraint, a child project depending on the banned package produces no violation. Only direct imports of the package are flagged, so the nested check does nothing.allowedExternalImportsconstraint and no ban list, the nested check is skipped entirely. A child project depending on a package outside the allowed list is never reported.Expected Behavior
Importing an internal project whose transitive external dependencies violate a
bannedExternalImportsorallowedExternalImportsconstraint reports a violation. The error names the violating package and the child project where it was found. Each package is reported once per child project. The allowed list is exclusive on the nested path, as it already is for direct imports.Related Issue(s)
Fixes #36519
Implementation Notes
isConstraintBanningProjectcompares the original import specifier against the external package name. On the nested path the specifier names the imported internal project while the compared package is the transitive external, so the guard never matched. Nested matching now uses each transitive package's own name. The direct import path keeps the specifier-based subpath matching.lodash/fpban does not flag a child depending onlodash. The rule options docs state this.allowedExternalImportsexisted. The gate accepts an allowed list too, including an empty one, which bans every external package.findTransitiveExternalDependencieskeeps one dependency edge per project and package. Several edges to the same package (static and dynamic, or several resolved versions) produce a single violation instead of identical repeats.View Polygraph session ↗