Skip to content
This repository has been archived by the owner on Jun 11, 2020. It is now read-only.

Make .allDependencyTypings() respect path mappings #779

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented May 8, 2020

If package A has a test dependency on B (because A's tests import B) and A has a path mapping B -> B/v#, then the compiler will use older-version B/v#, but AllPackages.allDependencyTypings() currently returns only the latest-versions of test dependencies:

for (const name of pkg.testDependencies) {
const versions = this.data.get(getMangledNameForScopedPackage(name));
if (versions) {
yield versions.getLatest();

This comes up in check-parse-results.ts checkPathMappings(), which checks for unused path mappings: All of a package's path mappings must be accounted for by a transitive dependency.

If A has a test dependency on B and a path mapping B -> B/v#, and B/v# in turn depends on C/v#, then A needs path mapping C -> C/v# or A's tests will combine possibly-incompatible packages B/v# and C-latest. But the compiler and checkPathMappings() currently use different versions of B: Unless B-latest also depends on C/v#, checkPathMappings() will complain that A's C -> C/v# path mapping is unused. Then A can't get past the CI automation: Its tests fail without the path mapping and checkPathMappings() fails with it.

checkPathMappings() already handles the case that A has a regular dependency on B. It respects the path mapping and uses the older version B/v#, same as the compiler. This is implemented in definition-parser.ts calculateDependencies():

dependencies.push({ name: dependencyName, version: pathMappingVersion });

This PR applies the same logic to AllPackages.allDependencyTypings() and test dependencies.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

This is a great fix. I've noticed this before and was very puzzled.

I have the same request as your other PR to move this fix and add a test by adding to createMockDT.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants