Skip to content

Conversation

sebws
Copy link

@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
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
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

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.

3 participants