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

fix path mapping checks for scoped and versioned packages #507

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

Conversation

jkillian
Copy link

Fixes #506

Fix how we parse path mappings for scoped and version packages.

Disclaimer: I wasn't sure how to fully test out my changes, please feel free to give things a run through. I did test it on the changes from DefinitelyTyped/DefinitelyTyped#29979 though and it seemed to resolve the issues.

@ghost ghost requested a review from sandersn November 1, 2018 23:20
// Path mapping may be for "@foo/bar" -> "foo__bar".
const scopedPackageName = unmangleScopedPackage(pathMapping);
// Path mapping may be for "@foo/bar" -> "foo__bar" or "@foo/bar" -> "foo__bar/v2"
const scopedPackageName = unmangleScopedPackage(stripVersion(pathMapping));
if (scopedPackageName !== undefined) {
if (dependencyName !== scopedPackageName) {
throw new Error(`Expected directory ${pathMapping} to be the path mapping for ${dependencyName}`);
Copy link
Member

Choose a reason for hiding this comment

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

calculateDependencies is odd in that only versioned packages need an entry in the dependencies it calculates. Previously, since we didn't check scoped packages for versions, the continue below was fine.

However, if you want to correctly handle versioned, scoped packages, then you'll have to move this section below the version-checking code that starts with const majorVersion = ... on line 294. Then that section will need to handle scoped packages correctly. I guess that the handling will be something like { name: scopedPackageName, majorVersion }, where scopedPackageName = unmangleScopedPackage(stripVersion(pathMapping)) (like you have above) and majorVersion = parseDependencyVersionFromPath(.... But you'll have to make sure that parseDependencyVersionFromPath handles scoped packages as well, perhaps by calling it with different arguments for the scoped version case.

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 may be a moot point since it sounds like you don't need @expo/vector-icons/v6 anyway, but the PR is currently wrong in that it treats @expo/vector-icons/v6 as if it were not versioned.

@Jessidhia
Copy link

This seems related to the problem I encountered in https://github.com/DefinitelyTyped/DefinitelyTyped/pull/30775/files#diff-269d3a25ff1729234b0bf52990c6c112 but I didn't check if this would fix it

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.

Scoped & versioned packages can't pass path mappings checks
3 participants