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

feat(cli): add lb4 update command to check/update project dependencies #4080

Merged
merged 3 commits into from
Nov 19, 2019

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Nov 8, 2019

Adding lb4 version for https://loopback.io/doc/en/lb4/Command-line-interface.html#upgrading-loopback-dependencies:

  • lb4 update runs against a LB4 project and checks dependencies against the installed @loopback/cli.
  • By default, lb4 update checks for exact match. Use --semver option to check compatibility using semver semantics.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@raymondfeng raymondfeng requested a review from bajtos as a code owner November 8, 2019 19:48
@raymondfeng raymondfeng force-pushed the lb4-upgrade-command branch 5 times, most recently from de24daa to f53d351 Compare November 9, 2019 05:45
@raymondfeng raymondfeng force-pushed the lb4-upgrade-command branch 2 times, most recently from cbc6ae9 to 3e288c6 Compare November 10, 2019 21:29
@MarkOfSine
Copy link

@raymondfeng this will be useful feature.
I would suggest an enhancement to include version checking of typescript. i.e. in particular noting where certain features are not yet compatible with a particular version of typescript.
Mainly forward looking (i.e. latest or more current versions of Typescript)

@raymondfeng
Copy link
Contributor Author

@MarkOfSine Thank you for chiming in.

I would suggest an enhancement to include version checking of typescript. i.e. in particular noting where certain features are not yet compatible with a particular version of typescript.
Mainly forward looking (i.e. latest or more current versions of Typescript)

The version of typescript is part of CLI template for dependencies too. As a result. lb4 version checks typescript. But we don't have enough knowledge to tell if it's fine to upgrade to latest version TypeScript though. Our experience is that it's usually safe to stay within patch versions, but often breaking with new minor. For example ~3.6.4 is good while ^3.6.4 is risky.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

👇


### Synopsis

The `lb4 version` command runs against a LoopBack 4 project and checks
Copy link
Member

Choose a reason for hiding this comment

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

I find the name version problematic, it does not communicate the fact that it can be used to update dependencies. We already have lb4 --version to print the version of LoopBack dependencies installed in the project.

I am proposing:

(1)
Improve lb4 --version to include information about packages that are outdated. At the moment, we print output like this:

$ lb4 --version
@loopback/cli version: 1.24.0

@loopback/* dependencies:
  - @loopback/authentication: ^3.2.1
  - @loopback/boot: ^1.5.10
...

I am proposing to add a short text after each outdated version, possibly highlighted using chalk.red:

$ lb4 --version
@loopback/cli version: 1.24.0

@loopback/* dependencies:
  - @loopback/authentication: ^3.0.1 (outdated)
  - @loopback/boot: ^1.5.10

(2)
Rename the new command to something like lb4 update or lb4 update-deps. Personally, I prefer lb4 update, because it's short and in line with npm update command that most developers are hopefully familiar with.

This opens a question though - what is the benefit of this LB-specific update command when compared to npm update? Why (or when?) should our users use this new command instead of npm update?

Copy link
Member

Choose a reason for hiding this comment

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

What is the benefit of this LB-specific update command when compared to npm update? Why (or when?) should our users use this new command instead of npm update?

It would be awesome if lb4 update could run scripts to automatically apply changes necessary to migrate the codebase to newer conventions. That would be a perfect explanation why to run lb4 update instead of npm update.

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'm good to have lb4 update. The key benefit of lb4 update over npm update is that lb4 update honors the collection of compatible LB4 packages that released with the current lb4 command from @loopback/cli.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please --version is implemented at cli.js level without a yeoman generator.

@raymondfeng raymondfeng changed the title feat(cli): add lb4 version command to check/upgrade project dependencies feat(cli): add lb4 update command to check/update project dependencies Nov 12, 2019
@raymondfeng
Copy link
Contributor Author

@bajtos I have refactored the code to use lb4 update.

@raymondfeng raymondfeng force-pushed the lb4-upgrade-command branch 4 times, most recently from 4881a7b to 6f0f35d Compare November 13, 2019 03:00
Copy link
Contributor

@emonddr emonddr left a comment

Choose a reason for hiding this comment

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

Cool feature.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Reviewed at high level, will review implementation details next week.

docs/site/Update-generator.md Outdated Show resolved Hide resolved
docs/site/Update-generator.md Show resolved Hide resolved
@raymondfeng
Copy link
Contributor Author

@bajtos PTAL.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I don't see any blocking problems, but please consider my comments below.

I am approving this PR to not block the progress, please get at least one person to approve the final patch before landing.

docs/site/Update-generator.md Outdated Show resolved Hide resolved
docs/site/Update-generator.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants