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: add upgrade command #8

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

feat: add upgrade command #8

wants to merge 5 commits into from

Conversation

bjohansebas
Copy link
Member

@bjohansebas bjohansebas commented Dec 3, 2024

This adds the command to update Express in a single step, displaying the available options based on the version specified in the package.json.

Things still to do:

  • Update the Express version in the package.json.
  • Make the jscodeshift input more user-friendly
  • add tests

@bjohansebas bjohansebas force-pushed the update-command branch 2 times, most recently from d83aa6a to b29fcef Compare December 7, 2024 20:26
@bjohansebas
Copy link
Member Author

@kjugi What do you think of the messages I added to the upgrade command? It no longer uses the jscodeshift output, although I think it's a bit confusing, the statistics output adds up the files in each loop.

console.info('> Cancelled process. Program will stop now without any actions. \n')
process.exit(1)
}
import { onCancel, promptSource } from '../utils/share'
Copy link
Member

Choose a reason for hiding this comment

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

this share filename needs adjustment. It does not tell what can be found inside. From what I can see you collect there utility methods for commands

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I gave it that name without thinking too much, I just wanted to separate it

commands/upgrade.ts Outdated Show resolved Hide resolved
const expressVersion = coerce(dependencies.express)?.version ?? '4.0.0'

const codemodsSuggested = TRANSFORM_OPTIONS.filter((a) => {
return compare(a.version, expressVersion) > 0
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we apply our codemods only if the project is on v4?

according to docs:

compare(v1, v2): Return 0 if v1 == v2, or 1 if v1 is greater, or -1 if v2 is greater.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is more thought out so that in the future more codemods can be added as Express evolves, and not limit us to version 4

{
type: 'multiselect',
name: 'codemodsSelected',
message: `The following 'codemods' are recommended for your upgrade. Select the ones to apply.`,
Copy link
Member

Choose a reason for hiding this comment

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

we recommend them only by checking version in the package.json file. It doesn't necessarily bring value. Let's just check if express is there and ask user to select codemods.

Copy link
Member Author

Choose a reason for hiding this comment

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

But what if there are codemods for version 6 and we're on version 5? We wouldn't want to recommend codemods from version 4, right?

console.log(`> ${results.ok} codemods were applied successfully`)
console.log(`> ${results.skipped} codemods were skipped`)
console.log(`> ${results.failed} codemods failed`)
console.log(`> ${results.unmodified} codemods were skipped because they didn't change anything`)
Copy link
Member

Choose a reason for hiding this comment

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

@bjohansebas
#8 (comment)

do you mean here?

Looks like a good summary after finishing the process.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep

@bjohansebas
Copy link
Member Author

I'm going to keep doing this next year, @kjugi if you want to keep going with it, it's okay by me.

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.

2 participants