fix path mapping checks for scoped and versioned packages#507
fix path mapping checks for scoped and versioned packages#507jkillian wants to merge 1 commit intomicrosoft:masterfrom
Conversation
ghost
requested a review
from 
| const scopedPackageName = unmangleScopedPackage(stripVersion(pathMapping)); | ||
| if (scopedPackageName !== undefined) { | ||
| if (dependencyName !== scopedPackageName) { | ||
| throw new Error(`Expected directory ${pathMapping} to be the path mapping for ${dependencyName}`); |
There was a problem hiding this comment.
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.
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
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.
|
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 |
3 participants
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.