Skip to content

fix: Add an update sub-command the @trigger.dev/cli that updates all @trigger.dev/* packages #317

Merged
ericallam merged 17 commits into
triggerdotdev:mainfrom
rishi-raj-jain:main
Aug 22, 2023
Merged

fix: Add an update sub-command the @trigger.dev/cli that updates all @trigger.dev/* packages #317
ericallam merged 17 commits into
triggerdotdev:mainfrom
rishi-raj-jain:main

Conversation

@rishi-raj-jain

Copy link
Copy Markdown

Resolves #166
/claim #166

@changeset-bot

changeset-bot Bot commented Aug 11, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4b64c2d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 25 packages
Name Type
@trigger.dev/cli Patch
@examples/job-catalog Patch
@examples/jobs-starter Patch
nextjs-12 Patch
@examples/nextjs Patch
@examples/package-tester Patch
perf Patch
@trigger.dev/tailwind-config Patch
@trigger.dev/tsconfig Patch
@trigger.dev/core Patch
@trigger.dev/eslint-plugin Patch
@trigger.dev/express Patch
@trigger.dev/integration-kit Patch
@trigger.dev/nextjs Patch
@trigger.dev/react Patch
@trigger.dev/sdk Patch
@trigger.dev/github Patch
@trigger.dev/openai Patch
@trigger.dev/plain Patch
@trigger.dev/resend Patch
@trigger.dev/sendgrid Patch
@trigger.dev/slack Patch
@trigger.dev/stripe Patch
@trigger.dev/supabase Patch
@trigger.dev/typeform Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment thread packages/cli/package.json Outdated
@rishi-raj-jain

rishi-raj-jain commented Aug 12, 2023

Copy link
Copy Markdown
Author

I've tested this with both npm & pnpm as well.

@ologbonowiwi would you like to confirm as well?

@wesleymatosdev wesleymatosdev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested here and it works locally

@rishi-raj-jain

Copy link
Copy Markdown
Author

Thanks for the confirmation! @ericallam Can you take a look at this now that we've confirmed working of this?

@matt-aitken matt-aitken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is mostly there. But there's an issue with how you're evaluating whether packages are the same or not.

You can test CLI changes like this by following from line 123 in the Contributions guide.

Comment thread packages/cli/src/commands/update.ts Outdated
Comment thread packages/cli/src/commands/update.ts Outdated
Comment thread packages/cli/src/commands/update.ts Outdated
@rishi-raj-jain

Copy link
Copy Markdown
Author

@matt-aitken @ericallam Did you get a chance to look at this one?

@matt-aitken

Copy link
Copy Markdown
Member

@rishi-raj-jain the code looks good, but I think we should use this package (recommended by another developer): https://www.npmjs.com/package/npm-check-updates

This would massively simplify the code here and will be more reliable as that package has a lot of contributors and high usage.

You can see how it's used as a package at the bottom of the readme. You can specify wildcards to get all the @trigger.dev/ packages.

I'll tip you generously if you make this change because we've messed you around a bit here. Sorry about that.

@rishi-raj-jain

Copy link
Copy Markdown
Author

Sure, I'll be on it. Appreciate your kindness ☺️

@rishi-raj-jain

rishi-raj-jain commented Aug 18, 2023

Copy link
Copy Markdown
Author

Hey @matt-aitken,

I just took a look at the suggested package.

For this won't the user need to install the npm-check-updates package on their device globally? I mean we can do it with another exec as well but it seems intrusive.
Also, in the environment that don't provide access to installing global packages would be affected?

@rishi-raj-jain

Copy link
Copy Markdown
Author

Happy to implement with the package suggested if you want me to. I'm fine with that as well. Just lmk.

@ericallam

Copy link
Copy Markdown
Member

@rishi-raj-jain You can use npm-check-updates programatically:

CleanShot 2023-08-21 at 16 16 56

So you just need to add npm-check-updates as a dep of the cli package and then use it in the code just like any other package.

@rishi-raj-jain

Copy link
Copy Markdown
Author

@ericallam

Thanks for pointing me in that direction!

This is ready for review.

Comment thread packages/cli/src/commands/update.ts Outdated
Comment thread packages/cli/src/commands/update.ts Outdated
Comment thread packages/cli/src/commands/update.ts Outdated
@ericallam

Copy link
Copy Markdown
Member

This is looking good! Can you please leave a comment on this PR the steps you've taken to test this change?

@rishi-raj-jain

Copy link
Copy Markdown
Author

@ericallam

Let me attach the whole reproduction video. I tried the whole thing in a seperate project.

@rishi-raj-jain

rishi-raj-jain commented Aug 22, 2023

Copy link
Copy Markdown
Author

Video: https://a.storyblok.com/f/117912/x/7b8a5ad330/screen-recording-2023-08-22-at-2-51-36-pm.mp4

  1. So in the video, I've copy pasted the whole code into a test folder which has latest axios and node-fetch.
  2. Then I downgrade axios & node-fetch to ^0 versions.
  3. As utilites in your project don't exist in my test environment, as you can see in the video, I've quickly added some code to read and write json file.
  4. Then I update the code to call the update command in the file (which in your case is mapped through the cli option).
  5. Then I run the function, it comes back with the message of no trigger.dev packages to update.
  6. Then I run the function after changing the selector to axios, and you can see it printing a table of old and new.
  7. Then I run the function after changing the selector to node-fetch, and you can see it printing a table of old and new.
  8. I press yes, and the package.json is updated.
  9. I expect the installDependencies to work with the utility you already have, hence haven't rewrote that in my test env.

@ericallam

Copy link
Copy Markdown
Member

Can you use our guide for testing CLI changes here:

https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md#testing-cli-changes

@rishi-raj-jain

Copy link
Copy Markdown
Author

@ericallam

Done, it worked! Here's the whole of video of the same:

Screen.Recording.2023-08-22.at.3.16.27.PM.mp4

@ericallam
ericallam merged commit 979f5f8 into triggerdotdev:main Aug 22, 2023
@rishi-raj-jain

Copy link
Copy Markdown
Author

@ericallam

Thanks for merging the PR.

#317 (comment)

Just saying that in that comment, I was told that I'd get a generous tip. Would be good to hear from you regarding the same.

@ericallam

Copy link
Copy Markdown
Member

/tip $100

@algora-pbc

algora-pbc Bot commented Aug 22, 2023

Copy link
Copy Markdown

🎉🎈 @rishi-raj-jain has been awarded $100! 🎈🎊

@rishi-raj-jain

Copy link
Copy Markdown
Author

Thank you so much for considering that, @ericallam! 🙏🏻

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.

[TRI-654] Add an update sub-command the @trigger.dev/cli that updates all @trigger.dev/* packages

4 participants