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

Claudiazhaoya · 2019-11-01T21:36:44Z

This address the issue: #991

…zhas/rush-add-test

.vscode/launch.json

apps/rush-lib/src/logic/PackageJsonUpdater.ts

common/config/rush/pnpm-lock.yaml

apps/rush-lib/src/logic/PackageJsonUpdater.ts

iclanton · 2019-11-01T21:50:07Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

+      const rushConfigProjects: RushConfigurationProject[] = this._rushConfiguration.projects;
+      rushConfigProjects.forEach(rushConfigProject => {
+        if(rushConfigProject.packageName === packageName) {
+          selectedVersion = rushConfigProject.packageJson.version;


You should break out of the loop here.

We also probably want to handle the case where a version is specified on the command-line and it doesn't match the version in the package.json

We also need to handle the case where the dependency that we're adding is marked as a cyclic dependency for the project in rush.json.

iclanton · 2019-11-01T21:50:28Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

@@ -355,18 +355,28 @@ export class PackageJsonUpdater {
      }
      console.log(`Querying NPM registry for latest version of "${packageName}"...`);

+      //if the project exists in the local


Suggested change

//if the project exists in the local

// determine if the package is a project in the local repository

iclanton · 2019-11-01T21:50:46Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

+          selectedVersion = rushConfigProject.packageJson.version;
+        }
+      });
+
      let commandArgs: string[];


This code should go inside the below condition.

common/changes/@microsoft/rush/zhas-rush-add-test_2019-11-01-21-43.json

iclanton · 2019-11-02T18:28:49Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

+      // determine if the package is a project in the local repository and if the version exists
+      for (const rushConfigProject of rushConfigProjects) {
+        const rushConfigProjectVersion: string = rushConfigProject.packageJson.version;
+        if(rushConfigProject.packageName === packageName && rushConfigProject.cyclicDependencyProjects.size === 0) {


The cyclic dependency check should probably happen before the loop starts.

I think this logic is also backwards: we need to check if the project we're adding the dependency to has the dependency listed as a cyclic. We also need to see if the dependency is in the set of cyclics, not just if there are any cyclic dependencies listed.

iclanton · 2019-11-02T18:30:04Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

+      for (const rushConfigProject of rushConfigProjects) {
+        const rushConfigProjectVersion: string = rushConfigProject.packageJson.version;
+        if(rushConfigProject.packageName === packageName && rushConfigProject.cyclicDependencyProjects.size === 0) {
+          if(initialSpec === rushConfigProjectVersion) {


This should use semver.satisfies(...). If the initialspec is anything other than a single version (like ~1.2.3), this will always cause an exception to be thrown.

apps/rush-lib/src/logic/PackageJsonUpdater.ts

iclanton · 2019-11-02T18:43:08Z

Can you list out the scenarios you tested? There are a lot of them and we should be sure the behavior is expected.

apostolisms · 2019-11-04T21:30:14Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

-      } else {
-        commandArgs = ['view', `${packageName}@latest`, 'version'];
+      // determine if the package is a project in the local repository
+      for (const rushConfigProject of rushConfigProjects) {


This seems to be duplicated above (very similar). Better to move to its own function

apostolisms · 2019-11-04T21:31:10Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

-        commandArgs = ['info', packageName, 'versions', '--json'];
-      } else {
-        commandArgs = ['view', packageName, 'versions', '--json'];
+      // determine if the package is a project in the local repository and if the version exists


Shouldn't we check for local packages after we checked the npm registry ?

If we find the local package, we don't need to check the registry.

iclanton · 2019-11-04T21:38:43Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

@@ -268,13 +269,15 @@ export class PackageJsonUpdater {
   * 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 project which will have its package.json updated


Suggested change

* @param projects - the project which will have its package.json updated

* @param projects - the projects which will have their `package.json`s updated

iclanton · 2019-11-04T21:39:19Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

-        commandArgs = ['info', packageName, 'versions', '--json'];
-      } else {
-        commandArgs = ['view', packageName, 'versions', '--json'];
+      // determine if the package is a project in the local repository and if the version exists


If we find the local package, we don't need to check the registry.

iclanton · 2019-11-04T21:41:26Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

+      // determine if the package is a project in the local repository and if the version exists
+      for (const rushConfigProject of rushConfigProjects) {
+        const rushConfigProjectVersion: string = rushConfigProject.packageJson.version;
+        if(rushConfigProject.packageName === packageName && !projects[0].cyclicDependencyProjects.has(packageName)) {


We don't just want to check the first project in the projects array. It should probably check all of them.

iclanton · 2019-11-04T21:41:37Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

   * @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[],


Do we support multiple projects?

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?

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

iclanton · 2019-11-04T21:46:30Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

+            selectedVersion = initialSpec;
+            break;
+          } else {
+            throw new Error(`Unable to find a version of "${packageName}" that satisfies`


Let's be a little more elaborate with this error:

Suggested change

throw new Error(`Unable to find a version of "${packageName}" that satisfies`

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

iclanton · 2019-11-04T21:58:44Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

+      // determine if the package is a project in the local repository
+      for (const rushConfigProject of rushConfigProjects) {
+        if(rushConfigProject.packageName === packageName && !projects[0].cyclicDependencyProjects.has(packageName)) {
+          if(selectedVersion === undefined) {


This check should go outside of the loop.

…zhas/rush-add-test

iclanton · 2019-11-06T01:11:06Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

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


Let's make these private to begin with.

iclanton · 2019-11-06T01:12:11Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

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


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

iclanton · 2019-11-06T01:12:44Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

+   * @param projects - the projects which will have their package.json's updated
+   * @param rushConfigProjects - the projects which are in the rush.json
+   */
+  public isLocalProject(


A better name for this would be getVersionForLocalDependency.

iclanton · 2019-11-06T01:14:12Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

      } else {
-        commandArgs = ['view', packageName, 'versions', '--json'];
+        throw new Error(`The dependency being added ("${packageName}") is a project in the local Rush repository,


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

iclanton · 2019-11-06T01:15:29Z

apps/rush-lib/src/logic/PackageJsonUpdater.ts

      } else {
-        commandArgs = ['view', packageName, 'versions', '--json'];
+        throw new Error(`The dependency being added ("${packageName}") is a project in the local Rush repository,


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

…zhas/rush-add-test

…idated as new depdndencies.

Claudiazhaoya added 4 commits October 25, 2019 09:54

add rush add test configuration

64ceb69

Merge branch 'master' of https://github.com/microsoft/rushstack into …

52f1136

…zhas/rush-add-test

fix rush add issue about local packages

58c5904

Merge branch 'master' of https://github.com/microsoft/rushstack into …

8965326

…zhas/rush-add-test

Claudiazhaoya requested review from apostolisms, D4N14L, halfnibble, iclanton, octogonz, patmill and sachinjoseph as code owners November 1, 2019 21:36

minor fix and add rush change file

f7a1739

iclanton requested changes Nov 1, 2019

View reviewed changes

Claudiazhaoya added 3 commits November 1, 2019 14:56

revert the changes made in launch.json

85c9c10

fix the comment =s

90cfe98

revert pnpm.yaml

85c986d

iclanton changed the title ~~Fix the issue: "rush add" should be able to add unpublished locally-linked projects~~ [rush] Fix the issue: "rush add" should be able to add unpublished locally-linked projects Nov 2, 2019

iclanton requested changes Nov 2, 2019

View reviewed changes

fix the comments

dc32432

apostolisms reviewed Nov 4, 2019

View reviewed changes

iclanton reviewed Nov 4, 2019

View reviewed changes

Claudiazhaoya added 3 commits November 4, 2019 18:02

fix comments except the elaborated error message

186ba48

Merge branch 'master' of https://github.com/microsoft/rushstack into …

c744202

…zhas/rush-add-test

add elaborated error message

62d6f1f

iclanton reviewed Nov 6, 2019

View reviewed changes

fix the comments

8964b1f

Claudiazhaoya and others added 9 commits November 6, 2019 15:25

Merge branch 'master' of https://github.com/microsoft/rushstack into …

d7300a1

…zhas/rush-add-test

minor fixs on syntax

f7cc78d

minor fix on comments

1d4893d

minor fix

ea770dd

Merge branch 'master' into zhas/rush-add-test

0be2e56

Clean up some issues with the way local projects are detected and val…

b20d97e

…idated as new depdndencies.

Fix a mistake in an error message.

27fae63

Simplfiy the cyclic dependency logic.

186a28e

Clean up error message.

cd47ff6

iclanton approved these changes Nov 21, 2019

View reviewed changes

iclanton merged commit 0c5c8d9 into master Nov 21, 2019

iclanton deleted the zhas/rush-add-test branch November 21, 2019 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Claudiazhaoya commented Nov 1, 2019 •

edited by rakeshpatnaik

Loading

iclanton Nov 1, 2019

iclanton Nov 1, 2019

iclanton Nov 1, 2019

iclanton Nov 2, 2019

iclanton Nov 2, 2019

iclanton commented Nov 2, 2019

apostolisms Nov 4, 2019

apostolisms Nov 4, 2019

iclanton Nov 4, 2019

iclanton Nov 4, 2019

iclanton Nov 4, 2019

iclanton Nov 4, 2019

iclanton Nov 4, 2019

Claudiazhaoya Nov 4, 2019

iclanton Nov 11, 2019

iclanton Nov 4, 2019

iclanton Nov 4, 2019

iclanton Nov 6, 2019

iclanton Nov 6, 2019

iclanton Nov 6, 2019

iclanton Nov 6, 2019

iclanton Nov 6, 2019

	//if the project exists in the local
	// determine if the package is a project in the local repository

	* @param projects - the project which will have its package.json updated
	* @param projects - the projects which will have their `package.json`s updated

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

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

Conversation

Claudiazhaoya commented Nov 1, 2019 • edited by rakeshpatnaik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iclanton commented Nov 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Claudiazhaoya commented Nov 1, 2019 •

edited by rakeshpatnaik

Loading