-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(node-core): passthrough node-cron context #17835
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
Conversation
4e0cb52 to
6f554fa
Compare
There was a problem hiding this 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 ?
Yea, we can add that in this PR.
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
}
}
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
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. (削除ここまで)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