-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
[Upgrade Tool] V2 #18990
[Upgrade Tool] V2 #18990
Conversation
…dependency is not found
…into upgrade-tool/init-package
…into upgrade-tool/init-package
Co-authored-by: Convly <jean-sebastien.herbaux@epitech.eu> Co-authored-by: Bassel Kanso <basselkanso82@gmail.com>
Co-authored-by: Ben Irvin <ben@innerdvations.com>
…n-git-repo.test.ts
…n-git-repo.test.ts
133b68e
to
a8f3a16
Compare
e85a23d
to
24dabd2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some of these comments are already outdated from the time I started the review.
Anyway, looks great, just a few comments!
const versionDirectory = path.join(this.cwd, literalVersion); | ||
|
||
// Ignore obsolete versions | ||
if (!fse.existsSync(versionDirectory)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to avoid sync methods, I've actually started removing them for v5 where possible and will eventually be adding a linting rule for them. But considering this is a CLI and every step is in series, it's not necessarily bad, so I can live with it (and it could ignore the future lint rule)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind using the async ones, I don't why I believed the performance hint was on async ones 🤔
(<> thought the io blocking was on the async)
}); | ||
}; | ||
|
||
addReleaseUpgradeCommand( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way, each type (major, minor, patch) has its own options and help command and has to import the upgrade.js for each. Are we really going to be having different options for major/minor/patch or could we just have one command with an argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we won't have different options between each command, but since the specs indicate 3 different paths, I prefer having them separated in 3 commands to handle possible divergences in the future + it doesn't cost us anything right now
@@ -0,0 +1,17 @@ | |||
export class UnexpectedError extends Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect to have other kinds of errors to be added here? I'm not sure why we need this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly it was just something I've bootstrapped now, but I plan on replacing every error with throw with custom ones so that it's easier to catch specific errors & understand where they're originating from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like that we've built our own timer, but it's pretty small I guess. We should eventually move it to @strapi/utils later and use it for other cli timers (like in pack-up, which has its own timer too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the idea, but honestly I didn't want this PR to become overcomplicated, we can handle refactoring of common utils in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some of these comments are already outdated from the time I started the review.
Anyway, looks great, just a few comments!
Closing in favor of #18995 @innerdvations could you transpose your comment there? Otherwise I can just answer them here and act on the other PR |
What does it do?
upgrade
Don't do anything and display the helpupgrade major
Will try upgrading to the next available major (only if the project is already on the latest minor.patch version for the current major), if none is found, it'll abortupgrade minor
todo in another PRupgrade patch
todo in another PRupgrade
packagesrc/cli/commands
contains actual commands' definitions, including the one for the upgradesrc/tasks
directory and is directly called by the upgrade commandsrc/modules/*
types.ts
exports types related to the moduleconstants.ts
exports constants related to the modulexyz.ts
contains domain logic related to the module (usually it exports a factory to create related domain objects. e.g:xyzFactory()
)index.ts
is considered the public module API and only exports what can be used by anyone outside the module__tests__/
directory which contains domain-related testsfs-extra
from10.0.0
to10.1.0
to fix compatibility issues withmemfs
which is used for mockingfs
during tests (see fs-extra's changelog and related issue / fix PR)Why is it needed?
After discussing at length the scope of the upgrade tool internally, we've decided to modify it a bit (mainly related to available commands, options, and how they should behave)
This PR aims to refactor the tool to incorporate those changes while cleaning up the overall architecture.
How to test it?
examples/getstarted
directory, use../../packages/utils/upgrade/bin/upgrade.js <command> [...options]
to test the upgrade CLI