Skip to content

Conversation

@Rutam21
Copy link
Contributor

@Rutam21 Rutam21 commented Oct 3, 2023

Closes #525

✅ Checklist

  • I have followed every step in the contributing guide
  • The PR title follows the convention.
  • I ran and tested the code works

Testing

Type checking has been done and no errors are found.

pnpm run --filter "./packages/**" typecheck

Testing

Smoke Testing has been done as suggested by @ericallam .

  • events job was run successfully and successful trigger was performed on the run. Find the screenshots below.

events

  • delays job was run successfully and successful trigger was performed on the run. Find the screenshots below.

delays

  • resend job was run successfully and successful trigger was performed on the run. Find the screenshots below.

resend

  • resend job was run successfully and successful trigger was performed on the run. Find the screenshots below.

Jobs were also run successfully on the Trigger.dev cloud dashboard.

jobs


Changelog

  • All the Packages are updated to use the Node v18.0.0 LTS from now on.
  • Certain other dependencies are updated using pnpm.
  • Node-Fetch imports and dependency are removed in all packages and fetch is getting used as the native function as supported by Node 18 now.

Screenshots

List of all modeified files are as follows.

File Change List

Note: The integrations also use the older Node versions. If they need to be updated as well, I would love to work on that ticket too as I am familiar with such issues now.

@changeset-bot
Copy link

changeset-bot bot commented Oct 3, 2023

⚠️ No Changeset found

Latest commit: 0558b2c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@Rutam21
Copy link
Contributor Author

Rutam21 commented Oct 3, 2023

@matt-aitken Please review and suggest changes, if any. Thanks

@ericallam
Copy link
Member

Have you run and tested the code still works? The easiest way to do that is to run jobs in references/job-catalog:

https://github.com/triggerdotdev/trigger.dev/blob/main/references/job-catalog/README.md

@Rutam21
Copy link
Contributor Author

Rutam21 commented Oct 4, 2023

Hello @ericallam, So, I need to run all jobs references/job-catalog one by one and check if they run without any errors or running some of them is enough?
Steps for that would be as follows:

  • Set .env file with the Trigger API Key
  • Build the CLI
  • Run each file in src
  • Then run the Trigger Dev Command fir each of them.

Please let me know if it's the right way. Thanks.

@ericallam
Copy link
Member

No definitely don't run them all haha, I would just pick a few to run as a smoke test. The simplest is probably events.ts

@Rutam21
Copy link
Contributor Author

Rutam21 commented Oct 4, 2023

Great, I will 3-4 files in the src and then update with the screenshots here.
So, all we are looking for now is that they run and there's no error in the terminal, right? Or do I need to look at some other paramters as well?

@ericallam
Copy link
Member

Yea considering the change, seeing successful runs should be a good enough test. And since this is a client only change, you don't even have to run the server locally if you don't want to, you could just use cloud.trigger.dev

@Rutam21
Copy link
Contributor Author

Rutam21 commented Oct 4, 2023

Perfect, I will update the screenshots asap.

@Rutam21
Copy link
Contributor Author

Rutam21 commented Oct 4, 2023

@ericallam I have performed the tests and updated the test results in the comments above. Please review and let me know if it looks good. Thanks.

Copy link
Member

@ericallam ericallam left a comment

Choose a reason for hiding this comment

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

Also make sure to run pnpm run typecheck in the root of the repo to make sure all the types still are valid.

},
"engines": {
"node": ">=16.8.0"
"node": "^18.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

This limits the Node version to >= 18 and < 19, which isn't what we want. I'd just do >=18.0.0

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.

[TRI-1333] [TRI-1332] Upgrade all packages/* and use fetch instead of node-fetch

2 participants