refactor!: Refactor cronjob controller to reduce duplication#3421
Merged
refactor!: Refactor cronjob controller to reduce duplication#3421
Conversation
Mrtenz
commented
May 26, 2025
| * @param params - The parameters for the background event. | ||
| * @returns The schedule parameters for the background event. | ||
| */ | ||
| function getStartDate(params: ScheduleBackgroundEventParams) { |
Member
Author
There was a problem hiding this comment.
The logic for scheduling was previously done in snaps-rpc-methods, but handling this from the cronjob controller is easier to reduce duplication.
Mrtenz
commented
May 26, 2025
Mrtenz
commented
May 26, 2025
| "jobs": [ | ||
| { | ||
| "expression": "*/10 * * * * *", | ||
| "expression": "PT10S", |
Member
There was a problem hiding this comment.
We need to find a way to test both formats 😓
Member
Author
There was a problem hiding this comment.
Likely easiest with a new example Snap. Just changed it here for local testing purposes.
794289e to
8951701
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3421 +/- ##
==========================================
+ Coverage 98.16% 98.18% +0.01%
==========================================
Files 402 399 -3
Lines 11141 11127 -14
Branches 1750 1750
==========================================
- Hits 10937 10925 -12
+ Misses 204 202 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
FrederikBolding
previously approved these changes
May 27, 2025
FrederikBolding
previously approved these changes
Jun 6, 2025
79d4de2 to
c16a656
Compare
FrederikBolding
previously approved these changes
Jun 6, 2025
Member
|
@Mrtenz Can you add a note about the breaking changes in the description please? |
FrederikBolding
approved these changes
Jun 6, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The cronjob controller contained a lot of essentially duplicated logic after the addition of background events. To resolve this, I've refactored the cronjob controller to treat cronjobs as recurring background events. This means:
lastRunproperty in favour of adateproperty. We can now simply check if the current date is past the scheduled date to know if a cronjob should be executed immediately.In addition to these changes, I've made some smaller changes to the controller like replacing TypeScript's
privatemodifier with true hash private functions.Breaking changes
jobsstate property was removed, cronjobs are now stored in theeventsproperty as well.CronjobController:schedulenow expects aschedulefield instead ofdate.BackgroundEventsuffix.CronjobController:scheduleBackgroundEvent->CronjobController:scheduleCronjobController:cancelBackgroundEvent->CronjobController:cancel.CronjobController:getBackgroundEvents->CronjobController:get.