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

Add "latest" flag to "yarn upgrade" #3510

Merged
merged 4 commits into from
Jun 7, 2017
Merged

Add "latest" flag to "yarn upgrade" #3510

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
02e0477
Added additional (passing) tests for existing upgrade behavior
rally25rs May 26, 2017
c48806c
[#3384] add --latest/-L flag to
rally25rs May 26, 2017
89fa8d4
merged master to branch upgrade-latest-parameter-3384
rally25rs Jun 7, 2017
5d79e03
added previously forgotten files to make tests actually pass
rally25rs Jun 7, 2017
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
64 changes: 64 additions & 0 deletions __tests__/commands/upgrade.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,70 @@ test.concurrent('upgrades from fixed version to latest', (): Promise<void> => {
});
});

test.concurrent('upgrades to latest matching package.json semver when no package name passed', (): Promise<void> => {
return runUpgrade([], {}, 'range-to-latest', async (config): ?Promise<void> => {
const lockfile = explodeLockfile(await fs.readFile(path.join(config.cwd, 'yarn.lock')));
const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
const lockEntryIndex = lockfile.indexOf('left-pad@<=1.1.1:');

expect(lockEntryIndex).toEqual(0);
expect(lockfile[lockEntryIndex + 1]).toContain('1.1.1');
expect(pkg.dependencies).toEqual({'left-pad': '<=1.1.1'});
});
});

test.concurrent('--latest upgrades to latest ignoring package.json when no package name passed', (): Promise<void> => {
return runUpgrade([], {latest: true}, 'range-to-latest', async (config): ?Promise<void> => {
const lockfile = explodeLockfile(await fs.readFile(path.join(config.cwd, 'yarn.lock')));
const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
const lockEntryIndex = lockfile.indexOf('left-pad@^1.1.3:');

expect(lockEntryIndex).toEqual(0);
expect(lockfile.indexOf('left-pad@<=1.1.1:')).toEqual(-1);
expect(lockfile[lockEntryIndex + 1]).toContain('1.1.3');
expect(pkg.dependencies).toEqual({'left-pad': '^1.1.3'});
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't use left-pad for this test, otherwise it will break if they release a new version. I think you can use yarn-test (cc @kittens ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I actually thought the packages used during tests were all loaded from the 'offline mirror' directory of __tests__/fixtures/request-cache/... but it does like like just the left-pad tgz files are in there, not the metadata. So all these tests are actually resolving real metadata? that seems fragile...

I needed a package that had enough diverse version numbers to cover all these upgrade cases, and left-pad was the first one I found that worked.

Is there someplace I can see what version of yarn-test exist?

(side-note, it might be useful to document how these tests actually work and should be constructed, for new developers who want to contribute. It took me a while the first time I needed to add one.)

Copy link
Member

Choose a reason for hiding this comment

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

So all these tests are actually resolving real metadata? that seems fragile...

Think so. That's why our tests practically never completely pass: we always end up timeouting at least one of them 😞 Probably should fix this at some point! I thought maybe we could set up some sort of fake registry with only the few endpoints required, that would extract its data from a repository local folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can discuss this in #3455

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The metadata for left-pad is taken from the __tests__/fixtures/request-cache/ directory, so a new version pushed to the live registry would not break the test. These tests actually pass with no internet connection.

});
});

test.concurrent('upgrades to latest matching semver when package name passed with version', (): Promise<void> => {
return runUpgrade(['left-pad@~1.1.2'], {}, 'range-to-latest', async (config): ?Promise<void> => {
const lockfile = explodeLockfile(await fs.readFile(path.join(config.cwd, 'yarn.lock')));
const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
const lockEntryIndex = lockfile.indexOf('left-pad@~1.1.2:');

expect(lockEntryIndex).toEqual(0);
expect(lockfile[lockEntryIndex + 1]).toContain('1.1.3');
expect(pkg.dependencies).toEqual({'left-pad': '~1.1.2'});
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 use left-pad here, but then use ~1.0.1 (should resolve to 1.0.2) instead of ~1.1.2, since we know there won't be any more release to this branch, which is not the case for 1.1.x.

});
});

test.concurrent('--latest upgrades to passed in version when package name passed with version', (): Promise<void> => {
return runUpgrade(['left-pad@1.1.2'], {latest: true}, 'range-to-latest', async (config): ?Promise<void> => {
const lockfile = explodeLockfile(await fs.readFile(path.join(config.cwd, 'yarn.lock')));
const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
const lockEntryIndex = lockfile.indexOf('left-pad@1.1.2:');

expect(lockEntryIndex).toEqual(0);
expect(lockfile.indexOf('left-pad@<=1.1.1:')).toEqual(-1);
expect(lockfile.indexOf('left-pad@^1.1.3:')).toEqual(-1);
expect(lockfile[lockEntryIndex + 1]).toContain('1.1.2');
expect(pkg.dependencies).toEqual({'left-pad': '1.1.2'});
});
});

test.concurrent('upgrades to latest ignoring package.json semver when package name passed', (): Promise<void> => {
return runUpgrade(['left-pad'], {}, 'range-to-latest', async (config): ?Promise<void> => {
const lockfile = explodeLockfile(await fs.readFile(path.join(config.cwd, 'yarn.lock')));
const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
const lockEntryIndex = lockfile.indexOf('left-pad@^1.1.3:');

expect(lockEntryIndex).toEqual(0);
expect(lockfile.indexOf('left-pad@<=1.1.1:')).toEqual(-1);
expect(lockfile[lockEntryIndex + 1]).toContain('1.1.3');
expect(pkg.dependencies).toEqual({'left-pad': '^1.1.3'});
});
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel right ... shouldn't we have to pass the --latest flag to have this behavior? It contradicts the "no argument means all packages" behavior ("upgrades to latest matching package.json semver when no package name passed").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test actually passes in current Yarn without my modifications (in other words, this is how Yarn has worked for a while). This is testing the command yarn upgrade left-pad, which is roughly equivalent to yarn add left-pad. The add command resolves to the passed in version range for left-pad which is unspecified, so it resolves to latest.

There is some discussion around that confusing behavior in 3384 3266 3204
I've always sort of wondered if it's an unintentional behavior.

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 just re-read my comment and decided it's confusing... In clearer terms, the way Yarn 0.24 - 0.26 all work currently is that if your package.json is:

"dependencies": {
  "left-pad": "^1.0.0"

Then yarn upgrade will upgrade left-pad to the greatest match for ^1.0.0.
However yarn upgrade left-pad will upgrade to latest.

I didn't change that behavior.
This PR just adds the option to:

yarn upgrade --latest which will upgrade left-pad to latest (ignoring what was in package.json)

There is currently, and with this PR, still no way to yarn upgrade <package_name> and have it use the range specified in package.json, unless you manually copy-paste the range yourself and do yarn upgrade left-pad@^1.0.0

Copy link
Member

@arcanis arcanis Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, you're right. I feel like this should be considered as a bug ... otherwise there's no way to upgrade a specific package inside its allowed boundaries :/ can you open a separate PR where we could discuss this?

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 spawned #3603 as a discussion issue and added you as an assignee.

});

test.concurrent('upgrades dependency packages not in registry', (): Promise<void> => {
const packages = ['yarn-test-git-repo', 'e2e-test-repo'];
return runUpgrade(packages, {}, 'package-not-in-registry', async (config): ?Promise<void> => {
Expand Down
9 changes: 8 additions & 1 deletion src/cli/commands/upgrade.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export function setFlags(commander: Object) {
// TODO: support some flags that install command has
commander.usage('upgrade [flags]');
commander.option('-S, --scope <scope>', 'upgrade packages under the specified scope');
commander.option('-L, --latest', 'upgrade packages to the latest version, ignoring version ranges in package.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need the -L shorthand? Let's just go with --latest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, was just following the convention of the other flags. I'll remove that -L.

}

export function hasWrapper(): boolean {
Expand All @@ -20,7 +21,8 @@ export function hasWrapper(): boolean {
export const requireLockfile = true;

export async function run(config: Config, reporter: Reporter, flags: Object, args: Array<string>): Promise<void> {
const lockfile = args.length ? await Lockfile.fromDirectory(config.cwd, reporter) : new Lockfile();
const useLockfile = args.length || flags.latest;
const lockfile = useLockfile ? await Lockfile.fromDirectory(config.cwd, reporter) : new Lockfile();
const {
dependencies,
devDependencies,
Expand Down Expand Up @@ -50,6 +52,11 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
} else {
throw new MessageError(reporter.lang('scopeNotValid'));
}
} else if (flags.latest && args.length === 0) {
addArgs = Object.keys(allDependencies)
.map(dependency => {
return getDependency(allDependencies, dependency);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you don't need the curly braces and the return here.

} else {
addArgs = args.map(dependency => {
return getDependency(allDependencies, dependency);
Expand Down