chore: Use DataSource for entity scanning (with pausing when the window loses focus) #1634
Conversation
This file contains 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
✅ Deploy Preview for kuma-gui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
johncowen
force-pushed
the
chore/refactor-data-source-usage
branch
8 times, most recently
from
561de7d to
962abde
Compare
johncowen
changed the title
kleinfreund
reviewed
Oct 30, 2023
johncowen
force-pushed
the
chore/refactor-data-source-usage
branch
from
a611e24 to
c7f14a6
Compare
|
I’ve looked over this again and am mostly fine with how this works. Two things I think need to be addressed as part of this PR (linking to the relevant threads) and not only in a follow up:
Could you also please rebase this PR? |
johncowen
force-pushed
the
chore/refactor-data-source-usage
branch
from
285cb2a to
6a78b62
Compare
Signed-off-by: John Cowen <[email protected]>
Signed-off-by: John Cowen <[email protected]>
Signed-off-by: John Cowen <[email protected]>
Signed-off-by: John Cowen <[email protected]>
Signed-off-by: John Cowen <[email protected]>
Signed-off-by: John Cowen <[email protected]>
Signed-off-by: John Cowen <[email protected]>
Signed-off-by: John Cowen <[email protected]>
Signed-off-by: John Cowen <[email protected]>
Signed-off-by: John Cowen <[email protected]>
Signed-off-by: John Cowen <[email protected]>
johncowen
force-pushed
the
chore/refactor-data-source-usage
branch
from
6a78b62 to
54433cb
Compare
kleinfreund
approved these changes
Nov 1, 2023
Signed-off-by: John Cowen <[email protected]>
This was referenced Nov 7, 2023
johncowen
added a commit
that referenced
this pull request
Nov 13, 2023
Back in #1634 we changed the functionality of checking for a connected zone by additionally catching a 404 error and retrying the request, which I'd wrongly assumed how this functioned. The endpoint only ever returns a 404 if the zone is deleted (i.e. doesn't exist). It doesn't 404 if the zone exists but is not connected. Due to the change, it was possible to create a zone, without connecting it, and then in say another tab, delete the zone, and the scanner would continue to spin due to the 404s. I've removed the 404 retries, and also added further testing for making sure a 404 shows the error page when the scanner is waiting to connect, and I also updated the mocks so that they are closer to the real responses. We decided that we should check for a `zone.enabled` (#1634 (comment)) but I don't think we ever need to check for `zone.enabled` for the purposes of this PR (or #1634) when checking for whether the zone is connected. I haven't altered anything there as yet as it doesn't have any effect, but I have updated the mocks here so that we don't get `enabled: true`, only a non-existant `enabled` (i.e. defaults to `true`) or `false`, which I 'think' is more correct. I re-added some comments explaining this a little better in a couple of places, and I almost renamed and moved around functions to help the code self-comment, but I'm a little way into a longer piece of work of collecting up zone data related functions de-duping them and refactoring them, which I'd rather continue to do separately rather than in this PR. The comments here will help when I continue with that. --------- Signed-off-by: John Cowen <[email protected]>
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.
Replaces the EntityScanner with a DataSource.
Makes all polling/retrying DataSources pause when the tab loses focus.
Whilst doing this I made a helper to let us configure the source of the DataSource easier from the sources.ts file so you can opt in to polling rather than opt out.
Closes #1582
Also addresses a part of #1474
I wanted to note that there will be some follow up work here, but I wanted to submit this PR so we could get this in rather than keep working on related things:
DataLoadercomponent I spoken about in the past.