fix path mapping checks for scoped and versioned packages #507
Conversation
ghost
requested a review
from 
| // 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}`); |
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.
|
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 |
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.