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.

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

[rush] Fix the issue: "rush add" should be able to add unpublished locally-linked projects #1612

Merged
merged 22 commits into from
Nov 21, 2019
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
64ceb69
add rush add test configuration
Claudiazhaoya Oct 25, 2019
52f1136
Merge branch 'master' of https://github.com/microsoft/rushstack into …
Claudiazhaoya Oct 25, 2019
58c5904
fix rush add issue about local packages
Claudiazhaoya Nov 1, 2019
8965326
Merge branch 'master' of https://github.com/microsoft/rushstack into …
Claudiazhaoya Nov 1, 2019
f7a1739
minor fix and add rush change file
Claudiazhaoya Nov 1, 2019
85c9c10
revert the changes made in launch.json
Claudiazhaoya Nov 1, 2019
90cfe98
fix the comment =s
Claudiazhaoya Nov 1, 2019
85c986d
revert pnpm.yaml
Claudiazhaoya Nov 2, 2019
dc32432
fix the comments
Claudiazhaoya Nov 4, 2019
186ba48
fix comments except the elaborated error message
Claudiazhaoya Nov 5, 2019
c744202
Merge branch 'master' of https://github.com/microsoft/rushstack into …
Claudiazhaoya Nov 5, 2019
62d6f1f
add elaborated error message
Claudiazhaoya Nov 5, 2019
8964b1f
fix the comments
Claudiazhaoya Nov 6, 2019
d7300a1
Merge branch 'master' of https://github.com/microsoft/rushstack into …
Claudiazhaoya Nov 6, 2019
f7cc78d
minor fixs on syntax
Claudiazhaoya Nov 7, 2019
1d4893d
minor fix on comments
Claudiazhaoya Nov 7, 2019
ea770dd
minor fix
Claudiazhaoya Nov 8, 2019
0be2e56
Merge branch 'master' into zhas/rush-add-test
iclanton Nov 11, 2019
b20d97e
Clean up some issues with the way local projects are detected and val…
iclanton Nov 11, 2019
27fae63
Fix a mistake in an error message.
iclanton Nov 11, 2019
186a28e
Simplfiy the cyclic dependency logic.
iclanton Nov 11, 2019
cd47ff6
Clean up error message.
iclanton Nov 11, 2019
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
149 changes: 111 additions & 38 deletions apps/rush-lib/src/logic/PackageJsonUpdater.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ export class PackageJsonUpdater {
);

const version: string = await this._getNormalizedVersionSpec(
projects,
installManager,
packageName,
initialVersion,
Expand Down Expand Up @@ -264,17 +265,65 @@ export class PackageJsonUpdater {
project.addOrUpdateDependency(packageName, newVersion, dependencyType!);
}

/**
* Check if the package is the cyclic dependency in the project dependencies in package.json
*/
public isCyclicDependency(
Copy link
Member

Choose a reason for hiding this comment

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

Let's make these private to begin with.

packageName: string,
projects: RushConfigurationProject[]
): boolean {
for (const project of projects) {
if(project.cyclicDependencyProjects.has(packageName)) {
return true;
}
}
return false;
}

/**
* Check if the project is local project
* If it is found in rush.json and is not a cyclic dependency, return true
Copy link
Member

Choose a reason for hiding this comment

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

This function returns a version specifier, not true or false.

* Otherwise, return false
* @param initialSpec - a semver pattern that should be used to find the latest version matching the spec
* @param packageName - the name of the package to be used
* @param projects - the projects which will have their package.json's updated
* @param rushConfigProjects - the projects which are in the rush.json
*/
public isLocalProject(
Copy link
Member

Choose a reason for hiding this comment

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

A better name for this would be getVersionForLocalDependency.

initialSpec: string | undefined,
packageName: string,
projects: RushConfigurationProject[],
rushConfigProjects: RushConfigurationProject[],
): string | undefined {
for (const rushConfigProject of rushConfigProjects) {
const rushConfigProjectVersion: string = rushConfigProject.packageJson.version;
const isCyclicDependency: boolean = this.isCyclicDependency(packageName, projects);
if(rushConfigProject.packageName === packageName && (!isCyclicDependency)) {
if(initialSpec !== undefined) {
if(semver.satisfies(rushConfigProjectVersion, initialSpec)) {
return initialSpec;
}
} else {
return rushConfigProjectVersion;
}
}
}
return undefined;
}

/**
* Selects an appropriate version number for a particular package, given an optional initial SemVer spec.
* If ensureConsistentVersions, tries to pick a version that will be consistent.
* Otherwise, will choose the latest semver matching the initialSpec and append the proper range style.
* @param projects - the projects which will have their package.json's updated
* @param packageName - the name of the package to be used
* @param initialSpec - a semver pattern that should be used to find the latest version matching the spec
* @param implicitlyPinnedVersion - the implicitly preferred (aka common/primary) version of the package in use
* @param rangeStyle - if this version is selected by querying registry, then this range specifier is prepended to
* the selected version.
*/
private async _getNormalizedVersionSpec(
projects: RushConfigurationProject[],
Copy link
Member

Choose a reason for hiding this comment

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

Do we support multiple projects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so.. That's why I'm curious about why it has an array here. The statement is like this: projects = [currentProject]; Don't you think we actually don't need this array here?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is used when the --all flag is specified.

installManager: InstallManager,
packageName: string,
initialSpec: string | undefined,
Expand Down Expand Up @@ -306,67 +355,91 @@ export class PackageJsonUpdater {

await installManager.ensureLocalPackageManager();
let selectedVersion: string | undefined;
const rushConfigProjects: RushConfigurationProject[] = this._rushConfiguration.projects;

if (initialSpec && initialSpec !== 'latest') {
iclanton marked this conversation as resolved.
Show resolved Hide resolved
console.log(colors.gray('Finding versions that satisfy the selector: ') + initialSpec);
console.log();
console.log(`Querying registry for all versions of "${packageName}"...`);

let commandArgs: string[];
if (this._rushConfiguration.packageManager === 'yarn') {
commandArgs = ['info', packageName, 'versions', '--json'];
// determine if the package is a project in the local repository and if the version exists
const version: string | undefined = this.isLocalProject(initialSpec, packageName, projects, rushConfigProjects);
if(version !== undefined) {
selectedVersion = version
} else {
commandArgs = ['view', packageName, 'versions', '--json'];
throw new Error(`The dependency being added ("${packageName}") is a project in the local Rush repository,
Copy link
Member

Choose a reason for hiding this comment

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

Split this onto multiple lines by concatenating strings. The way this is written, there will be line breaks and indentation in the error message.

Copy link
Member

Choose a reason for hiding this comment

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

This will throw if the dependency being added isn't a local dependency. In that case, we want to just query the feed.

but the version specifier provided (${initialSpec}) does not match the local project version (${version}).
Correct the version specifier, omit a version specifier, or include "${packageName}" as a cyclicDependencyProject in rush.json
if it is intended for "${packageName}"
to come from an external feed and not from the local Rush repository."`);
}

const allVersions: string =
Utilities.executeCommandAndCaptureOutput(
this._rushConfiguration.packageManagerToolFilename,
commandArgs,
this._rushConfiguration.commonTempFolder
);
if(selectedVersion === undefined) {
console.log(`Querying registry for all versions of "${packageName}"...`);

let versionList: string[];
if (this._rushConfiguration.packageManager === 'yarn') {
versionList = JSON.parse(allVersions).data;
} else {
versionList = JSON.parse(allVersions);
}
let commandArgs: string[];
if (this._rushConfiguration.packageManager === 'yarn') {
commandArgs = ['info', packageName, 'versions', '--json'];
} else {
commandArgs = ['view', packageName, 'versions', '--json'];
}

console.log(colors.gray(`Found ${versionList.length} available versions.`));
const allVersions: string =
Utilities.executeCommandAndCaptureOutput(
this._rushConfiguration.packageManagerToolFilename,
commandArgs,
this._rushConfiguration.commonTempFolder
);

let versionList: string[];
if (this._rushConfiguration.packageManager === 'yarn') {
versionList = JSON.parse(allVersions).data;
} else {
versionList = JSON.parse(allVersions);
}

for (const version of versionList) {
if (semver.satisfies(version, initialSpec)) {
selectedVersion = initialSpec;
console.log(`Found a version that satisfies ${initialSpec}: ${colors.cyan(version)}`);
break;
console.log(colors.gray(`Found ${versionList.length} available versions.`));

for (const version of versionList) {
if (semver.satisfies(version, initialSpec)) {
selectedVersion = initialSpec;
console.log(`Found a version that satisfies ${initialSpec}: ${colors.cyan(version)}`);
break;
}
}
}

if (!selectedVersion) {
throw new Error(`Unable to find a version of "${packageName}" that satisfies`
+ ` the version specifier "${initialSpec}"`);
if (!selectedVersion) {
throw new Error(`Unable to find a version of "${packageName}" that satisfies`
+ ` the version specifier "${initialSpec}"`);
}
}
} else {
if (!this._rushConfiguration.ensureConsistentVersions) {
console.log(colors.gray(`The "ensureConsistentVersions" policy is NOT active,`
+ ` so we will assign the latest version.`));
console.log();
}
console.log(`Querying NPM registry for latest version of "${packageName}"...`);

let commandArgs: string[];
if (this._rushConfiguration.packageManager === 'yarn') {
commandArgs = ['info', packageName, 'dist-tags.latest', '--silent'];
} else {
commandArgs = ['view', `${packageName}@latest`, 'version'];
const version: string | undefined = this.isLocalProject(initialSpec, packageName, projects, rushConfigProjects);
if(version !== undefined) {
selectedVersion = version
}

selectedVersion = Utilities.executeCommandAndCaptureOutput(
this._rushConfiguration.packageManagerToolFilename,
commandArgs,
this._rushConfiguration.commonTempFolder
).trim();
if (selectedVersion === undefined) {
console.log(`Querying NPM registry for latest version of "${packageName}"...`);

let commandArgs: string[];
if (this._rushConfiguration.packageManager === 'yarn') {
commandArgs = ['info', packageName, 'dist-tags.latest', '--silent'];
} else {
commandArgs = ['view', `${packageName}@latest`, 'version'];
}

selectedVersion = Utilities.executeCommandAndCaptureOutput(
this._rushConfiguration.packageManagerToolFilename,
commandArgs,
this._rushConfiguration.commonTempFolder
).trim();
}

console.log();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "@microsoft/rush",
"comment": "Fix an issue with Rush add, where Rush was unable to add unpublished local projects as dependencies.",
"type": "none"
}
],
"packageName": "@microsoft/rush",
"email": "[email protected]"
}
2 changes: 1 addition & 1 deletion rush.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
* The default value is false to avoid legacy compatibility issues.
* It is strongly recommended to set strictPeerDependencies=true.
*/
"strictPeerDependencies": true,
"strictPeerDependencies": true


/**
Expand Down