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] Introduce a "rush add" command #843

Merged
merged 32 commits into from
Sep 28, 2018
Merged

[rush] Introduce a "rush add" command #843

merged 32 commits into from
Sep 28, 2018

Conversation

nickpape
Copy link
Contributor

Introduces a rush add command which can change dependencies.

  • The command should be run when inside a project folder.
  • The command requires the name of the new dependency to be added with --package/-p
  • If no version is specified, it will use the latest version, unless ensureConsistentVersions policy is active, in which case it will pick a version that will not violate the policy.
  • If a version range is specified (--version), the newest version that satisfies the range will be selected.
    • The new version will be prepended with ~ by default. If --caret is used, then it will be prepended with ^. If --exact is specified, nothing will be prepended.
  • The --make-consistent flag must be used if the new version introduces a version mismatch and
    the ensureConsistentVersions policy is active.
    • This command will update all packages that use the dependency to be consistent.
  • rush update will be run after the command unless --no-install is specified.

@nickpape
Copy link
Contributor Author

nickpape commented Sep 26, 2018

Fixes #820 #Resolved

@nickpape
Copy link
Contributor Author

nickpape commented Sep 26, 2018

@pgonzal @iclanton #Resolved

@nickpape
Copy link
Contributor Author

nickpape commented Sep 26, 2018

Example:
image

#Resolved

required: true,
argumentName: 'PACKAGE_NAME',
description: '(Required) The name of the package which should be added as a dependency'
});
Copy link
Member

@iclanton iclanton Sep 26, 2018

Choose a reason for hiding this comment

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

This is clunky. How hard would it be to add an "anonymous" argument to TS-command-line? #WontFix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pete and I discussed this, but it will be a pretty big change in ts-command-line. We decided to just go with the normal way for now, then potentially change it later.


In reply to: 220407271 [](ancestors = 220407271)

parameterShortName: '-v',
argumentName: 'VERSION_RANGE',
description: ''
});
Copy link
Member

@iclanton iclanton Sep 26, 2018

Choose a reason for hiding this comment

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

Can this be specified as package@version? #WontFix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be, but requires extra parsing and whatnot. We will make it like that when we allow anonymous arguments.


In reply to: 220407345 [](ancestors = 220407345)

Copy link
Member

Choose a reason for hiding this comment

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

Don't we already have parsing for that?


In reply to: 220743394 [](ancestors = 220743394,220407345)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I am aware of...


In reply to: 220749976 [](ancestors = 220749976,220743394,220407345)

Copy link
Member

Choose a reason for hiding this comment

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

Can't we just split by the "@" character?


In reply to: 220751648 [](ancestors = 220751648,220749976,220743394,220407345)

description: 'If specified, the version specifier inserted into the'
+ ' package.json will be a prepended with a "caret" specifier ("^").'
});
this._devDependencyFlag = this.defineFlagParameter({
Copy link
Member

@iclanton iclanton Sep 26, 2018

Choose a reason for hiding this comment

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

These can be just specified with the version. #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

?


In reply to: 220407415 [](ancestors = 220407415)

Copy link
Member

Choose a reason for hiding this comment

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

You can specify the version as "~1.2.3" or "^2.3.4"


In reply to: 220743430 [](ancestors = 220743430,220407415)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You highlighted devDependency flag.

And that isn't how --version works. It works how "npm install --save" works.


In reply to: 220750046 [](ancestors = 220750046,220743430,220407415)

Copy link
Member

Choose a reason for hiding this comment

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

So NPM has --carat and --tilde arguments? That seems really clunky.


In reply to: 220751772 [](ancestors = 220751772,220750046,220743430,220407415)

});
this._makeConsistentFlag = this.defineFlagParameter({
parameterLongName: '--make-consistent',
parameterShortName: '-c',
Copy link
Member

@iclanton iclanton Sep 26, 2018

Choose a reason for hiding this comment

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

-c [](start = 27, length = 2)

-m? #Resolved

});
this._noInstallFlag = this.defineFlagParameter({
parameterLongName: '--no-install',
parameterShortName: '-n',
Copy link
Member

@iclanton iclanton Sep 26, 2018

Choose a reason for hiding this comment

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

-n [](start = 27, length = 2)

Is this the same as npm? #WontFix

Copy link
Member

Choose a reason for hiding this comment

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

Wontfix?


In reply to: 220407520 [](ancestors = 220407520)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean. NPM doesn't have an option for --skip-install or --no-install or anything.


In reply to: 220750062 [](ancestors = 220750062,220407520)

Copy link
Member

Choose a reason for hiding this comment

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

That's what I was asking.


In reply to: 220751842 [](ancestors = 220751842,220750062,220407520)

parameterLongName: '--version',
parameterShortName: '-v',
argumentName: 'VERSION_RANGE',
description: ''
Copy link
Member

@iclanton iclanton Sep 26, 2018

Choose a reason for hiding this comment

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

'' [](start = 19, length = 2)

Include a description. #Resolved


constructor(parser: RushCommandLineParser) {
const documentation: string[] = [
'Blah.'
Copy link
Member

@iclanton iclanton Sep 26, 2018

Choose a reason for hiding this comment

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

Blah [](start = 7, length = 4)

? #Resolved

@@ -743,6 +743,16 @@ export class RushConfiguration {
return this._versionPolicyConfiguration;
}

public getCurrentProjectFromPath(currentFolderPath: string): RushConfigurationProject | undefined {
Copy link
Member

@iclanton iclanton Sep 26, 2018

Choose a reason for hiding this comment

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

getCurrentProjectFromPath [](start = 9, length = 25)

getProjectFromPath? It doesn't necessarily need to be the current project or the current path. #Resolved

public getCurrentProjectFromPath(currentFolderPath: string): RushConfigurationProject | undefined {
const resolvedPath: string = path.resolve(currentFolderPath);
for (const project of this.projects) {
if (path.relative(project.projectFolder, resolvedPath).indexOf('..') !== 0) {
Copy link
Member

@iclanton iclanton Sep 26, 2018

Choose a reason for hiding this comment

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

path.relative(project.projectFolder, resolvedPath).indexOf('..') !== 0 [](start = 10, length = 70)

use Path.isUnder from node-core-library. #Resolved

@@ -156,6 +156,8 @@ class RushConfiguration {
readonly eventHooks: EventHooks;
findProjectByShorthandName(shorthandProjectName: string): RushConfigurationProject | undefined;
findProjectByTempName(tempProjectName: string): RushConfigurationProject | undefined;
// (undocumented)
getCurrentProjectFromPath(currentFolderPath: string): RushConfigurationProject | undefined;
Copy link
Member

@iclanton iclanton Sep 26, 2018

Choose a reason for hiding this comment

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

Add docs. #Resolved

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this the "current" project? Probably it should be getProjectForPath?


In reply to: 220408115 [](ancestors = 220408115)

public getCurrentProjectFromPath(currentFolderPath: string): RushConfigurationProject | undefined {
const resolvedPath: string = path.resolve(currentFolderPath);
for (const project of this.projects) {
if (path.relative(project.projectFolder, resolvedPath).indexOf('..') !== 0) {
Copy link
Collaborator

@octogonz octogonz Sep 26, 2018

Choose a reason for hiding this comment

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

path.relative [](start = 10, length = 13)

Use Path.isUnder #Resolved

[--dev] [-c] [-n]


Blah.
Copy link
Collaborator

@octogonz octogonz Sep 26, 2018

Choose a reason for hiding this comment

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

? #Resolved

Copy link
Collaborator

Choose a reason for hiding this comment

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

Document that it's a tilde by default.


In reply to: 220731693 [](ancestors = 220731693)

(Required) The name of the package which should be
added as a dependency
-v VERSION_RANGE, --version VERSION_RANGE
--exact If specified, the version specifier inserted into the
Copy link
Collaborator

@octogonz octogonz Sep 26, 2018

Choose a reason for hiding this comment

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

inserted into [](start = 59, length = 14)

"SemVer specifier added to" #Resolved

-c, --make-consistent
If specified, other packages with this dependency
will have their package.json files updated to point
at the specified depdendency
Copy link
Collaborator

@octogonz octogonz Sep 26, 2018

Choose a reason for hiding this comment

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

specify the same SemVer range? #Resolved

If specified, other packages with this dependency
will have their package.json files updated to point
at the specified depdendency
-n, --no-install If specified, the \\"rush update\\" command will not be
Copy link
Collaborator

@octogonz octogonz Sep 26, 2018

Choose a reason for hiding this comment

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

no-install [](start = 8, length = 10)

--skip-update? #Resolved

import { PurgeManager } from './PurgeManager';
import { Utilities } from '../utilities/Utilities';

export const enum SemVerStyle {
Copy link
Collaborator

@octogonz octogonz Sep 26, 2018

Choose a reason for hiding this comment

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

export [](start = 0, length = 6)

Add docs in this file #Resolved

@@ -0,0 +1,219 @@
import * as semver from 'semver';
Copy link
Collaborator

@octogonz octogonz Sep 28, 2018

Choose a reason for hiding this comment

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

import [](start = 0, length = 7)

copyright banner #Resolved

* Returns the project for which the specified path is underneath that project's folder.
* If the path is not under any project's folder, returns undefined.
*/
public getProjectForPath(currentFolderPath: string): RushConfigurationProject | undefined {
Copy link
Collaborator

@octogonz octogonz Sep 28, 2018

Choose a reason for hiding this comment

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

tryGet? #Resolved

*/
public get packageJson(): IPackageJson {
return this._packageJson;
}

/**
* A useful wrapper around the package.json file for making modifications
* @alpha
Copy link
Collaborator

@octogonz octogonz Sep 28, 2018

Choose a reason for hiding this comment

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

@beta -- we don't use alpha in WBT #Resolved

@@ -172,11 +176,20 @@ export class RushConfigurationProject {

/**
* The parsed NPM "package.json" file from projectFolder.
* Will be deprecated soon in favor of packageJsonEditor
Copy link
Collaborator

@octogonz octogonz Sep 28, 2018

Choose a reason for hiding this comment

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

Will [](start = 5, length = 4)

@deprecated Use packageJsonEditor instead #Resolved



Adds a dependency on a certain package to the current project (detected using
the current working directory) and then runs rush update. If no version is
Copy link
Collaborator

@octogonz octogonz Sep 28, 2018

Choose a reason for hiding this comment

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

rush update [](start = 45, length = 11)

"rush update" #Resolved

latest version or a version that won't break the ensureConsistentVersions
policy). If a version range is specified, the latest version in the range
will be used. The version will be automatically prepended with a tilde,
unless the --exact or --caret flags are used. The --make-consistent flag can
Copy link
Collaborator

@octogonz octogonz Sep 28, 2018

Choose a reason for hiding this comment

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

--exact [](start = 11, length = 7)

"--exact" #Resolved

package.json will be a prepended with a \\"caret\\"
specifier (\\"^\\").
-d, --dev If specified, the package will be added as a
\\"devDependency\\" to the package.json
Copy link
Collaborator

@octogonz octogonz Sep 28, 2018

Choose a reason for hiding this comment

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

\"devDependency\" to the package.json [](start = 23, length = 40)

to the package.json "devDependencies" section #Resolved

@@ -202,6 +203,9 @@ class RushConfigurationProject {
// @beta
readonly isMainProject: boolean;
readonly packageJson: IPackageJson;
// WARNING: The type "PackageJsonEditor" needs to be exported by the package (e.g. added to index.ts)
Copy link
Collaborator

@octogonz octogonz Sep 28, 2018

Choose a reason for hiding this comment

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

PackageJsonEditor [](start = 24, length = 17)

Fix this warning #Resolved

if (mismatches.length) {
if (!updateOtherPackages) {
return Promise.reject(new Error(`Adding '${packageName}@${version}' to ${currentProject.packageName}`
+ ` causes mismatched dependencies. Use the --make-consistent flag to update other packages to use this`
Copy link
Collaborator

@octogonz octogonz Sep 28, 2018

Choose a reason for hiding this comment

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

--make-consistent [](start = 56, length = 17)

"--make-consistent" #Resolved

Copy link
Collaborator

@octogonz octogonz left a comment

Choose a reason for hiding this comment

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

:shipit:

@nickpape nickpape merged commit 6587a80 into master Sep 28, 2018
@octogonz octogonz deleted the nickpape/rush-add branch October 3, 2018 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants