Skip to content

Conversation

@sebws
Copy link
Contributor

@sebws sebws commented Oct 1, 2025

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

- [ ] If you've added code that should be tested, please add tests.

  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

node-cron passes context to the functions which this integration does not.

the types here seem to be largely irrelevant as consumers just get the actual node-cron type. however, it might be better to install node-cron as a devdependency and then import and use types from it to get consistency

@sebws sebws force-pushed the fix-node-cron-integration branch from 4e0cb52 to 6f554fa Compare October 1, 2025 01:03
@chargome chargome requested a review from andreiborza October 1, 2025 09:49
Copy link
Member

@chargome chargome left a comment

Choose a reason for hiding this comment

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

Makes sense to me, should we add a dev dep for this @andreiborza ?

@andreiborza
Copy link
Member

Yea, we can add that in this PR.

@sebws
Copy link
Contributor Author

sebws commented Oct 3, 2025

tbh it might be a bit harder, given this wrapper already wraps incorrectly and doesn't support node-cron fully, e.g. string referring to a file instead of a callback.

however, just for fun, perhaps it would be more accurate to do

"peerDependencies": { "node-cron": "^4" }, "peerDependenciesMeta": { "node-cron": { "optional": true } } 
@sebws
Copy link
Contributor Author

sebws commented Oct 3, 2025

maybe a different style of wrapper would be more accurate that passed through without manipulation and would add event listeners, leaving node-cron to fully handle the execution.

although I'm not sure if that would work with how withMonitor works

@andreiborza
Copy link
Member

Right, okay then let's keep it as is.

Thanks for the contribution!

@andreiborza andreiborza merged commit 4b18ffd into getsentry:develop Dec 11, 2025
118 checks passed
nicohrubec added a commit that referenced this pull request Dec 11, 2025
This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #17835 Co-authored-by: andreiborza <168741329+andreiborza@users.noreply.github.com> Co-authored-by: Nicolas Hrubec <nico.hrubec@sentry.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants