Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Take into account peerDependency versions when calculating packageId #57029

Merged
merged 9 commits into from
Apr 1, 2024
Merged

Take into account peerDependency versions when calculating packageId #57029

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f5d0ef0
Baseline
sheetalkamat Jan 11, 2024
98e72b0
Handle peer dependencies in pnpm layout
sheetalkamat Jan 11, 2024
3c7612e
Update Baselines and/or Applied Lint Fixes
typescript-bot Jan 11, 2024
3d33069
oops
sheetalkamat Jan 11, 2024
9b1d076
Merge branch 'main' into pnpmFileVersioning
sheetalkamat Jan 22, 2024
9497cf8
Always read peer dependencies
sheetalkamat Jan 22, 2024
dfefb0e
Merge branch 'main' into pnpmFileVersioning
sheetalkamat Feb 22, 2024
6de58fc
Merge branch 'main' into pnpmFileVersioning
sheetalkamat Mar 29, 2024
620bb48
Make API internal
sheetalkamat Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5455,6 +5455,18 @@
"category": "Message",
"code": 6280
},
"'package.json' has a 'peerDependencies' field.": {
"category": "Message",
"code": 6281
},
"Found peerDependency '{0}' with '{1}' version.": {
"category": "Message",
"code": 6282
},
"Failed to find peerDependency '{0}'.": {
"category": "Message",
"code": 6283
},

"Enable project compilation": {
"category": "Message",
Expand Down
57 changes: 46 additions & 11 deletions src/compiler/moduleNameResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ export function isTraceEnabled(compilerOptions: CompilerOptions, host: ModuleRes
return !!compilerOptions.traceResolution && host.trace !== undefined;
}

function withPackageId(packageInfo: PackageJsonInfo | undefined, r: PathAndExtension | undefined): Resolved | undefined {
function withPackageId(packageInfo: PackageJsonInfo | undefined, r: PathAndExtension | undefined, state: ModuleResolutionState): Resolved | undefined {
let packageId: PackageId | undefined;
if (r && packageInfo) {
const packageJsonContent = packageInfo.contents.packageJsonContent as PackageJson;
Expand All @@ -129,14 +129,15 @@ function withPackageId(packageInfo: PackageJsonInfo | undefined, r: PathAndExten
name: packageJsonContent.name,
subModuleName: r.path.slice(packageInfo.packageDirectory.length + directorySeparator.length),
version: packageJsonContent.version,
peerDependencies: getPeerDependenciesOfPackageJsonInfo(packageInfo, state),
};
}
}
return r && { path: r.path, extension: r.ext, packageId, resolvedUsingTsExtension: r.resolvedUsingTsExtension };
}

function noPackageId(r: PathAndExtension | undefined): Resolved | undefined {
return withPackageId(/*packageInfo*/ undefined, r);
return withPackageId(/*packageInfo*/ undefined, r, /*state*/ undefined!); // State will not be used so no need to pass
}

function removeIgnoredPackageId(r: Resolved | undefined): PathAndExtension | undefined {
Expand Down Expand Up @@ -346,6 +347,7 @@ export interface PackageJsonPathFields {
interface PackageJson extends PackageJsonPathFields {
name?: string;
version?: string;
peerDependencies?: MapLike<string>;
}

function readPackageJsonField<TMatch, K extends MatchingKeys<PackageJson, string | undefined>>(jsonContent: PackageJson, fieldName: K, typeOfTag: "string", state: ModuleResolutionState): PackageJson[K] | undefined;
Expand Down Expand Up @@ -669,7 +671,7 @@ export function resolveTypeReferenceDirective(typeReferenceDirectiveName: string
if (resolvedFromFile) {
const packageDirectory = parseNodeModuleFromPath(resolvedFromFile.path);
const packageInfo = packageDirectory ? getPackageJsonInfo(packageDirectory, /*onlyRecordFailures*/ false, moduleResolutionState) : undefined;
return resolvedTypeScriptOnly(withPackageId(packageInfo, resolvedFromFile));
return resolvedTypeScriptOnly(withPackageId(packageInfo, resolvedFromFile, moduleResolutionState));
}
}
return resolvedTypeScriptOnly(
Expand Down Expand Up @@ -1984,7 +1986,7 @@ function nodeLoadModuleByRelativeName(extensions: Extensions, candidate: string,
if (resolvedFromFile) {
const packageDirectory = considerPackageJson ? parseNodeModuleFromPath(resolvedFromFile.path) : undefined;
const packageInfo = packageDirectory ? getPackageJsonInfo(packageDirectory, /*onlyRecordFailures*/ false, state) : undefined;
return withPackageId(packageInfo, resolvedFromFile);
return withPackageId(packageInfo, resolvedFromFile, state);
}
}
if (!onlyRecordFailures) {
Expand Down Expand Up @@ -2201,7 +2203,7 @@ function loadNodeModuleFromDirectory(extensions: Extensions, candidate: string,
const packageInfo = considerPackageJson ? getPackageJsonInfo(candidate, onlyRecordFailures, state) : undefined;
const packageJsonContent = packageInfo && packageInfo.contents.packageJsonContent;
const versionPaths = packageInfo && getVersionPathsOfPackageJsonInfo(packageInfo, state);
return withPackageId(packageInfo, loadNodeModuleFromDirectoryWorker(extensions, candidate, onlyRecordFailures, state, packageJsonContent, versionPaths));
return withPackageId(packageInfo, loadNodeModuleFromDirectoryWorker(extensions, candidate, onlyRecordFailures, state, packageJsonContent, versionPaths), state);
}

/** @internal */
Expand Down Expand Up @@ -2373,6 +2375,8 @@ export interface PackageJsonInfoContents {
versionPaths: VersionPaths | false | undefined;
/** false: resolved to nothing. undefined: not yet resolved */
resolvedEntrypoints: string[] | false | undefined;
/** false: peerDependencies are not present. undefined: not yet resolved */
peerDependencies: string | false | undefined;
}

/**
Expand Down Expand Up @@ -2400,6 +2404,37 @@ function getVersionPathsOfPackageJsonInfo(packageJsonInfo: PackageJsonInfo, stat
return packageJsonInfo.contents.versionPaths || undefined;
}

function getPeerDependenciesOfPackageJsonInfo(packageJsonInfo: PackageJsonInfo, state: ModuleResolutionState): string | undefined {
if (packageJsonInfo.contents.peerDependencies === undefined) {
packageJsonInfo.contents.peerDependencies = readPackageJsonPeerDependencies(packageJsonInfo, state) || false;
}
return packageJsonInfo.contents.peerDependencies || undefined;
}

function readPackageJsonPeerDependencies(packageJsonInfo: PackageJsonInfo, state: ModuleResolutionState): string | undefined {
const peerDependencies = readPackageJsonField(packageJsonInfo.contents.packageJsonContent, "peerDependencies", "object", state);
if (peerDependencies === undefined) return undefined;
if (state.traceEnabled) trace(state.host, Diagnostics.package_json_has_a_peerDependencies_field);
const packageDirectory = realPath(packageJsonInfo.packageDirectory, state.host, state.traceEnabled);
const nodeModules = packageDirectory.substring(0, packageDirectory.lastIndexOf("node_modules") + "node_modules".length) + directorySeparator;
let result = "";
for (const key in peerDependencies) {
if (hasProperty(peerDependencies, key)) {
const peerPackageJson = getPackageJsonInfo(nodeModules + key, /*onlyRecordFailures*/ false, state);
if (peerPackageJson) {
const version = (peerPackageJson.contents.packageJsonContent as PackageJson).version;
result += `+${key}@${version}`;
Copy link
Member

@weswigham weswigham Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Q: I could theoretically name a package on disk typescript@5.4.1+ and munge this cache key up pretty badly (all these separators are valid filepath characters after all). Shouldn't this use, eg, ? and | instead of + and @, since, at least on windows, those aren't valid path characters? Believe it or not, a require("typescript@5.4.1+") will work and resolve on disk, even if npm won't let you publish a package with those characters!

Copy link
Member Author

Choose a reason for hiding this comment

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

But that shouldnt matter right becaus this is just distinguisher and gets added as packageName@version+peerPackage@peerPackageversion... as the package ID for distinguishing.. Actually was wondering if peerPackageDependency should be array of { packageName, version} instead so that in future we can use it say in module specifier etc .

Copy link
Member Author

Choose a reason for hiding this comment

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

@andrewbranch do you think we would be able to use peer dependency to determine the "moduleSpecifier" name for transitive dependencies? If yes i will make change to make this object with packageName and version instead of a string of all peer dependencies together?

Copy link
Member

Choose a reason for hiding this comment

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

Where would we use that exactly? Usually when we’re trying to compute a module specifier, we’re starting from a source file or module symbol. So I’m not sure where this would come in.

Copy link
Member Author

Choose a reason for hiding this comment

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

eg. if your file has import whose peer dependency is the source File for the package then short cut to directly use package name?

Copy link
Member Author

Choose a reason for hiding this comment

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

import { a } from "foo";
export functopn bar() { 
return a.bar(); // This is from package Bar@BarVersion
}

then while going through all imports in the file: if you find say "foo" import whose peer dependency is "Bar@Bar.version" use that package name ?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe that could be leveraged somehow, but getting just the package name is usually fairly easy compared to figuring out what subpath can be used to get to the export we need.

Copy link
Member Author

Choose a reason for hiding this comment

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

@weswigham @andrewbranch do we still have concern about the string for peer dependency calculations.
Do we want this as array of package name and versions for future use or just some string is ok. Should i make this internal for now so we have option to change this later? Would like to get this in sooner than later

Copy link
Member

Choose a reason for hiding this comment

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

I think it should probably be internal if it’s a formatted string that only gets used as a lookup id. But if you want to go ahead and change it to be an object with name/version, that works too.

if (state.traceEnabled) trace(state.host, Diagnostics.Found_peerDependency_0_with_1_version, key, version);
}
else {
// Read the dependency version
if (state.traceEnabled) trace(state.host, Diagnostics.Failed_to_find_peerDependency_0, key);
}
}
}
return result;
}

/** @internal */
export function getPackageJsonInfo(packageDirectory: string, onlyRecordFailures: boolean, state: ModuleResolutionState): PackageJsonInfo | undefined {
const { host, traceEnabled } = state;
Expand Down Expand Up @@ -2430,7 +2465,7 @@ export function getPackageJsonInfo(packageDirectory: string, onlyRecordFailures:
if (traceEnabled) {
trace(host, Diagnostics.Found_package_json_at_0, packageJsonPath);
}
const result: PackageJsonInfo = { packageDirectory, contents: { packageJsonContent, versionPaths: undefined, resolvedEntrypoints: undefined } };
const result: PackageJsonInfo = { packageDirectory, contents: { packageJsonContent, versionPaths: undefined, resolvedEntrypoints: undefined, peerDependencies: undefined } };
if (state.packageJsonInfoCache && !state.packageJsonInfoCache.isReadonly) state.packageJsonInfoCache.setPackageJsonInfo(packageJsonPath, result);
state.affectingLocations?.push(packageJsonPath);
return result;
Expand Down Expand Up @@ -2768,7 +2803,7 @@ function getLoadModuleFromTargetImportOrExport(extensions: Extensions, state: Mo
const finalPath = toAbsolutePath(pattern ? resolvedTarget.replace(/\*/g, subpath) : resolvedTarget + subpath);
const inputLink = tryLoadInputFileForPath(finalPath, subpath, combinePaths(scope.packageDirectory, "package.json"), isImports);
if (inputLink) return inputLink;
return toSearchResult(withPackageId(scope, loadFileNameFromPackageJsonField(extensions, finalPath, /*onlyRecordFailures*/ false, state)));
return toSearchResult(withPackageId(scope, loadFileNameFromPackageJsonField(extensions, finalPath, /*onlyRecordFailures*/ false, state), state));
}
else if (typeof target === "object" && target !== null) { // eslint-disable-line no-null/no-null
if (!Array.isArray(target)) {
Expand Down Expand Up @@ -2914,7 +2949,7 @@ function getLoadModuleFromTargetImportOrExport(extensions: Extensions, state: Mo
if (!extensionIsOk(extensions, possibleExt)) continue;
const possibleInputWithInputExtension = changeAnyExtension(possibleInputBase, possibleExt, ext, !useCaseSensitiveFileNames(state));
if (state.host.fileExists(possibleInputWithInputExtension)) {
return toSearchResult(withPackageId(scope, loadFileNameFromPackageJsonField(extensions, possibleInputWithInputExtension, /*onlyRecordFailures*/ false, state)));
return toSearchResult(withPackageId(scope, loadFileNameFromPackageJsonField(extensions, possibleInputWithInputExtension, /*onlyRecordFailures*/ false, state), state));
}
}
}
Expand Down Expand Up @@ -3050,7 +3085,7 @@ function loadModuleFromSpecificNodeModulesDirectory(extensions: Extensions, modu
packageInfo.contents.packageJsonContent,
getVersionPathsOfPackageJsonInfo(packageInfo, state),
);
return withPackageId(packageInfo, fromDirectory);
return withPackageId(packageInfo, fromDirectory, state);
}

const loader: ResolutionKindSpecificLoader = (extensions, candidate, onlyRecordFailures, state) => {
Expand All @@ -3073,7 +3108,7 @@ function loadModuleFromSpecificNodeModulesDirectory(extensions: Extensions, modu
// a default `index.js` entrypoint if no `main` or `exports` are present
pathAndExtension = loadModuleFromFile(extensions, combinePaths(candidate, "index.js"), onlyRecordFailures, state);
}
return withPackageId(packageInfo, pathAndExtension);
return withPackageId(packageInfo, pathAndExtension, state);
};

if (rest !== "") {
Expand Down Expand Up @@ -3273,7 +3308,7 @@ function resolveFromTypeRoot(moduleName: string, state: ModuleResolutionState) {
if (resolvedFromFile) {
const packageDirectory = parseNodeModuleFromPath(resolvedFromFile.path);
const packageInfo = packageDirectory ? getPackageJsonInfo(packageDirectory, /*onlyRecordFailures*/ false, state) : undefined;
return toSearchResult(withPackageId(packageInfo, resolvedFromFile));
return toSearchResult(withPackageId(packageInfo, resolvedFromFile, state));
}
const resolved = loadNodeModuleFromDirectory(Extensions.Declaration, candidate, !directoryExists, state);
if (resolved) return toSearchResult(resolved);
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7741,6 +7741,7 @@ export interface PackageId {
subModuleName: string;
/** Version of the package, e.g. "1.2.3" */
version: string;
peerDependencies?: string;
}

export const enum Extension {
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,7 @@ export function createModuleNotFoundChain(sourceFile: SourceFile, host: TypeChec
}

function packageIdIsEqual(a: PackageId | undefined, b: PackageId | undefined): boolean {
return a === b || !!a && !!b && a.name === b.name && a.subModuleName === b.subModuleName && a.version === b.version;
return a === b || !!a && !!b && a.name === b.name && a.subModuleName === b.subModuleName && a.version === b.version && a.peerDependencies === b.peerDependencies;
}

/** @internal */
Expand All @@ -819,7 +819,7 @@ export function packageIdToPackageName({ name, subModuleName }: PackageId): stri

/** @internal */
export function packageIdToString(packageId: PackageId): string {
return `${packageIdToPackageName(packageId)}@${packageId.version}`;
return `${packageIdToPackageName(packageId)}@${packageId.version}${packageId.peerDependencies ?? ""}`;
}

/** @internal */
Expand Down
Loading
Loading