Refactor: Precise control over data fetching and task execution#1334
Merged
jcscottiii merged 1 commit intomainfrom Mar 31, 2025
Merged
Refactor: Precise control over data fetching and task execution#1334jcscottiii merged 1 commit intomainfrom
jcscottiii merged 1 commit intomainfrom
Conversation
Background: This PR refactors data fetching in two components by turning autoRun to false. I originally observed an issue in Firefox in my original PR (#1330). The firefox browser was seeing the tasks trigger multiple times and I thought my current handling of TaskNotReadyError and DuplicateErrors/TaskNotReadyError would handle it. However, in lit task if a task triggers while there's a pending task already running, it would abort the first task. So as a result, sometimes firefox would not get any information if the original task was aborted and the subsequent tasks were returning one of the DuplicateTaskErrors/TaskNotReadyError causing no more task executions to fully succeed. This tries to simplify things by turning off autoRun and leveraging the willUpdate lit lifecycle method to precisely run a task. This allows us to not have to do checks on the task's arguments within the task itself since we control when the task exactly triggers. Main changes: - **`OverviewPage`:** The `loadingTask` is no longer set to `autoRun: true`. Instead, it is manually triggered in `willUpdate` based on changes to relevant properties and bookmark loading status. The `TaskNotReadyError` and related logic have been removed. - **`webstatus-bookmarks-service`:** - The `loadingUserSavedBookmarkByIDTask` is no longer set to `autoRun: true` and is triggered in `willUpdate`. - The task now returns an object containing both the bookmark data and the current search query. Other changes: - **`webstatus-firebase-auth-service`:** Updated the type of the `user` state to `User | null | undefined`. - Removed some redundant tests. Changed some tests to use sinon stubs - Added some more coverage too. This is a split up of #1330
This was referenced Mar 29, 2025
Collaborator
I think you are missing the word "user" in that part of the PR description. |
Collaborator
Author
Thanks! |
jrobbins
approved these changes
Mar 31, 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
2 participants
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.
Background:
This PR refactors data fetching in two components by disabling
autoRunfor Lit Tasks. An issue was observed in Firefox in PR #1330 where tasks were being triggered multiple times. The previous error handling usingTaskNotReadyErrorand checks for duplicate tasks was insufficient. Lit Task aborts a currently pending task if a new one is triggered. This led to scenarios in Firefox where the initial task could be aborted, and subsequent tasks might return errors, preventing successful data retrieval.This tries to simplify things by turning off autoRun and leveraging the willUpdate lit lifecycle method to precisely run a task. This also allows us to not have to do checks on the task's arguments within the task itself since we control when the task exactly triggers.
Main changes:
OverviewPage: TheloadingTaskis no longer set toautoRun: true. Instead, it is manually triggered inwillUpdatebased on changes to relevant properties and bookmark loading status. TheTaskNotReadyErrorand related logic have been removed.webstatus-bookmarks-service:loadingUserSavedBookmarkByIDTaskis no longer set toautoRun: trueand is triggered inwillUpdate.Other changes:
webstatus-firebase-auth-service: Updated the type of theuserstate toUser | null | undefined. This is useful for noting whether some data about the user has been loaded successfully or not vs no decision about the user has been made.Other notes:
This is a split up of #1330