chore: Use DataSource for entity scanning (with pausing when the wind…

…ow loses focus) (#1634) 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: - I 'think' its a good point to PR the `DataLoader` component I spoken about in the past. - The DataSource file/import organization is a bit messy just down to how DataSource came over here, I'd like to make that a bit less messy. - Probably PR #1525 for real. --------- Signed-off-by: John Cowen <[email protected]>
kumahq · Nov 1, 2023 · c1a4331 · c1a4331
1 parent e35828e
commit c1a4331
Show file tree

Hide file tree

Showing 11 changed files with 259 additions and 292 deletions.
diff --git a/features/zones/Create.feature b/features/zones/Create.feature
@@ -16,7 +16,8 @@ Feature: zones / create
       | environment-kubernetes-config       | [data-testid='zone-kubernetes-config']               |
       | ingress-input-switch                | [for='zone-ingress-enabled']                         |
       | egress-input-switch                 | [for='zone-egress-enabled']                          |
-      | zone-connected-scanner              | [data-testid='zone-connected-scanner']               |
+      | waiting                             | [data-testid='waiting']                              |
+      | connected                           | [data-testid='connected']                            |
       | error                               | [data-testid='create-zone-error']                    |
       | instructions                        | [data-testid='connect-zone-instructions']            |
     And the environment
@@ -68,57 +69,59 @@ Feature: zones / create
       """
       KUMA_SUBSCRIPTION_COUNT: 0
       """
+    And the URL "/provision-zone" responds with
+      """
+      body:
+        token: spat_595QOxTSreRmrtdh8ValuoeUAzXMfBmRwYU3V35NQvwgLAWIU
+      """
     When I visit the "/zones/-create" URL
     Then the "$create-zone-button" element is disabled
 
     When I "type" "test" into the "$name-input" element
     Then the "$create-zone-button" element isn't disabled
 
-    When the URL "/provision-zone" responds with
-      """
-      body:
-        token: spat_595QOxTSreRmrtdh8ValuoeUAzXMfBmRwYU3V35NQvwgLAWIU
-      """
-    And I click the "$create-zone-button" element
+    When I click the "$create-zone-button" element
     Then the URL "/provision-zone" was requested with
       """
       method: POST
       body:
         name: test
       """
-    Then the "$environment-universal-radio-button" element isn't checked
-    Then the "$environment-kubernetes-radio-button" element is checked
-    Then the "$ingress-input-switch input" element is checked
-    Then the "$egress-input-switch input" element is checked
-    Then the "$environment-kubernetes-config" element contains "kdsGlobalAddress: grpcs://<global-kds-address>:5685"
-    Then the "$zone-connected-scanner[data-test-state='waiting']" element exists
+    And the "$environment-universal-radio-button" element isn't checked
+    And the "$environment-kubernetes-radio-button" element is checked
+    And the "$ingress-input-switch input" element is checked
+    And the "$egress-input-switch input" element is checked
+    And the "$environment-kubernetes-config" element contains "kdsGlobalAddress: grpcs://<global-kds-address>:5685"
+    And the "$waiting" element exists
 
     When I click the "$ingress-input-switch" element
     Then the "$ingress-input-switch input" element isn't checked
-    Then the "$egress-input-switch input" element is checked
+    And the "$egress-input-switch input" element is checked
 
     When I click the "$egress-input-switch" element
     Then the "$ingress-input-switch input" element isn't checked
-    Then the "$egress-input-switch input" element isn't checked
+    And the "$egress-input-switch input" element isn't checked
 
     When I click the "$environment-universal-radio-button + label" element
     Then the "$ingress-input-switch input" element doesn't exist
-    Then the "$egress-input-switch input" element doesn't exist
-    Then the "$environment-universal-config" element contains "globalAddress: grpcs://<global-kds-address>:5685"
+    And the "$egress-input-switch input" element doesn't exist
+    And the "$environment-universal-config" element contains "globalAddress: grpcs://<global-kds-address>:5685"
 
     Given the environment
       """
       KUMA_SUBSCRIPTION_COUNT: 1
       """
-    When the URL "/zones/test/_overview" responds with
+    And the URL "/zones/test/_overview" responds with
       """
       body:
+        zone:
+          enabled: true
         zoneInsight:
           subscriptions:
             - connectTime: '2020-07-28T16:18:09.743141Z'
               disconnectTime: !!js/undefined
       """
-    Then the "$zone-connected-scanner[data-test-state='success']" element exists
+    Then the "$connected" element exists
 
   Scenario: The form shows expected error for 409 response
     Given the URL "/provision-zone" responds with
@@ -186,6 +189,8 @@ Feature: zones / create
     And the URL "/zones/test/_overview" responds with
       """
       body:
+        zone:
+          enabled: true
         zoneInsight:
           subscriptions:
             - connectTime: '2020-07-28T16:18:09.743141Z'
@@ -197,7 +202,7 @@ Feature: zones / create
     And I click the "$create-zone-button" element
 
     Then the "$instructions" element exists
-    And the "$zone-connected-scanner[data-test-state='success']" element exists
+    And the "$connected" element exists
 
     When I click the "$exit-button" element
 
@@ -220,7 +225,7 @@ Feature: zones / create
     And I click the "$create-zone-button" element
 
     Then the "$instructions" element exists
-    And the "$zone-connected-scanner[data-test-state='waiting']" element exists
+    And the "$waiting" element exists
 
     When I click the "$exit-button" element
 

diff --git a/src/app/application/index.ts b/src/app/application/index.ts
@@ -7,14 +7,17 @@ import RouteView from './components/route-view/RouteView.vue'
 import { routes } from './routes'
 import can from './services/can'
 import I18n from './services/i18n/I18n'
+import DataSourceLifeCycle, { getSource } from '@/app/application/services/data-source'
+import type { Source } from '@/app/application/services/data-source'
 import { DataSourcePool } from '@/app/application/services/data-source/DataSourcePool'
-import DataSourceLifeCycle from '@/app/application/services/data-source/index'
 import type { EnvVars } from '@/services/env/Env'
 import Env from '@/services/env/Env'
 import type { ServiceDefinition } from '@/services/utils'
-import { token, createInjections } from '@/services/utils'
+import { token, createInjections, constant } from '@/services/utils'
 import type { Component } from 'vue'
 
+export type { DataSourceResponse, Source } from './services/data-source'
+
 type Can = ReturnType<typeof can>
 type Token = ReturnType<typeof token>
 
@@ -39,6 +42,7 @@ const $ = {
   notFoundView: token<() => Promise<Component>>('application.not-found'),
   applicationComponents: token('application.components'),
 
+  source: token<Source>('data.source'),
   sources: token('data.sources'),
   dataSourcePool: token<DataSourcePool>('data.DataSourcePool'),
   dataSourceLifecycle: token<typeof DataSourceLifeCycle>('data.DataSourceLifecycle'),
@@ -114,6 +118,11 @@ export const services = (app: Record<string, Token>): ServiceDefinition[] => {
       constant: DataSourceLifeCycle,
     }],
 
+    [$.source, {
+      service: getSource,
+      arguments: [constant(document, { description: 'dom.document' })],
+    }],
+
     [$.getDataSourceCacheKeyPrefix, {
       service: () => () => '',
       arguments: [

diff --git a/src/app/application/services/data-source/CallableEventSource.ts b/src/app/application/services/data-source/CallableEventSource.ts
@@ -4,7 +4,8 @@ const CLOSED = 2
 export const isClosed = (source: { readyState: number }) => source.readyState === CLOSED
 // CallableEventSource turns a Promise returning function into an EventTarget,
 // making it act like a standard EventSource.
-export default class CallableEventSource extends EventTarget {
+
+export default class CallableEventSource<T = {}> extends EventTarget {
   url = ''
   withCredentials = false
   readonly CONNECTING = CONNECTING
@@ -18,7 +19,7 @@ export default class CallableEventSource extends EventTarget {
 
   constructor(
     protected source: () => AsyncGenerator,
-    _configuration = {},
+    public configuration: T,
   ) {
     super()
     this._open()
@@ -42,6 +43,7 @@ export default class CallableEventSource extends EventTarget {
         // temporarily commented until we can avoid console.errors being
         // reported in environments where we don't want to see them
         // console.error(e)
+        self.close()
         self.dispatchEvent(new ErrorEvent('error', {
           error: e,
         }))

diff --git a/src/app/application/services/data-source/index.ts b/src/app/application/services/data-source/index.ts
@@ -1,36 +1,97 @@
-import CallableEventSource, { isClosed } from './CallableEventSource'
+import CallableEventSource from './CallableEventSource'
 import type { Creator, Destroyer } from './DataSourcePool'
+export type { DataSourceResponse } from './DataSourcePool'
+
+type Configuration = {
+  interval?: number
+  retry?: (e: unknown) => Promise<void> | undefined
+}
+type RetryingEventSource = CallableEventSource<Configuration>
+type Hideable = EventTarget & { hidden: boolean }
+
+export const getSource = (doc: Hideable) => {
+  return (cb: (source: RetryingEventSource) => Promise<unknown>, config: Configuration = {}) => {
+    let attempts = 0
+    let iterations = 0
+    return new CallableEventSource<Configuration>(async function * (this: RetryingEventSource) {
+      const self = this
+      while (true) {
+        self.readyState = 1
+        // this this isn't the first call then we should wait before calling again
+        if (iterations > 0) {
+          await new Promise((resolve) => setTimeout(resolve, self.configuration.interval ?? 1000))
+        }
+        if (attempts > 0 || iterations > 0) {
+          // if the document/browser tab is hidden then wait for it to regain
+          // focus but, for the first call (if we aren't erroring) we probably
+          // still want to send of the request, so then at least when you come
+          // back you immediately see data
+          if (doc.hidden) {
+            await new Promise((resolve) => {
+              doc.addEventListener('visibilitychange', resolve, { once: true })
+            })
+          }
+        }
+        let res
+        try {
+          res = cb(self)
+          // if we aren't polling then immediately close after calling
+          if (typeof self.configuration.interval === 'undefined') {
+            self.close()
+          }
+          // only increase iterations if we didn't error
+          iterations++
+          // return the result
+          yield res
+        } catch (e) {
+          // if retry is configured await it before entering the loop again to
+          // try again
+          // TODO(jc): we should probably pass through attempts and maybe other
+          // things here
+          const retry = self.configuration?.retry?.(e)
+          if (typeof retry?.then === 'function') {
+            // make sure we never mistakenly retry sooner than 1s
+            await Promise.all([retry, new Promise(resolve => setTimeout(resolve, 1000))])
+            attempts++
+          } else {
+            throw e
+          }
+        }
+      }
+    }, config)
+  }
+}
+export type Source = ReturnType<typeof getSource>
+
+// its fine to not wait for an unfocussed tab for promise returning sources
+const source = getSource(new (class extends EventTarget {hidden = false})())
 
 export const create: Creator = (src, router) => {
   const [path, query] = src.split('?')
   const queryParams = new URLSearchParams(query)
   // use the router to find which function to call
   const route = router.match(path)
-  const _source = new CallableEventSource(async function * (this: CallableEventSource) {
-    while (true) {
-      this.readyState = 1
-      // `.route` here is the function call to the 'source' i.e. the Promise
-      // returning call that can be polled, in our case right now the HTTP
-      // calls but in the future could also be 'listeners' on localStorage, or
-      // 'listeners' on a session
-      yield route.route({
-        ...{
-          offset: parseInt(queryParams.get('offset') || '0'),
-          size: parseInt(queryParams.get('size') || '0'),
-          page: parseInt(queryParams.get('page') || '0'),
-          search: queryParams.get('search') || '',
-        },
-        ...route.params,
-      }, this)
-      if (!isClosed(this)) {
-        // right now any polling has a 5s interval, we currently just hardcode
-        // here but if/when we use this more this should be a user setting
-        // that we can save/retrieve from localStorage
-        await new Promise(resolve => setTimeout(resolve, 5000))
-      }
+  const params = {
+    ...{
+      offset: parseInt(queryParams.get('offset') || '0'),
+      size: parseInt(queryParams.get('size') || '0'),
+      page: parseInt(queryParams.get('page') || '0'),
+      search: queryParams.get('search') || '',
+    },
+    ...route.params,
+  }
+  try {
+    // TODO(jc) Once we remove all the source.closes in the sources.ts files the
+    // second argument here can go
+    const init = route.route(params, { close: () => {} })
+    if (init instanceof CallableEventSource) {
+      return init
+    } else {
+      return source(() => Promise.resolve(init))
     }
-  })
-  return _source
+  } catch (e) {
+    return source(() => Promise.reject(e))
+  }
 }
 export const destroy: Destroyer = (_src, source) => {
   if (source) {