[Logs UI] Remove apollo deps from log link-to routes #74502
Conversation
weltenwort
added
v8.0.0
Feature:Logs UI
|
@elasticmachine merge upstream |
This was removed in a non-backwards-compatible way in elastic#61162.
|
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
| @@ -45,6 +47,10 @@ export const useLogSource = ({ | |||
| return await callFetchLogSourceConfigurationAPI(sourceId, fetch); | |||
| }, | |||
| onResolve: ({ data }) => { | |||
| if (!getIsMounted()) { | |||
There was a problem hiding this comment.
I assume this fixes the race condition you mentioned in the description.
Could you help me understand what was the problem? I see that useMountedState is a wrapper around useEffect. I looked for usages of these promise creators and I couldn't find uses that weren't wrapped in useEffect already.
I worry that this might introduce a "gotcha" if consumers of this API need to call the promises before their component is mounted (i.e. in useLayoutEffect). If I understand this correctly, calling the promise will do nothing.
There was a problem hiding this comment.
useMountedState returns a predicate that indicates whether the component is mounted. This becomes important when the component unmounts while the promise is pending. Then, when the promise is resolved, the subsequent setState will cause an exception. Therefore all asynchronously called setStates should be guarded by such a check. I'm still trying to catch up with all the occurrences of that mistake in this plugin, so this is a small step.
There was a problem hiding this comment.
I worry that this might introduce a "gotcha" if consumers of this API need to call the promises before their component is mounted (i.e. in
useLayoutEffect). If I understand this correctly, calling the promise will do nothing.
As described above, the promise itself is not the problem. It resolving and thereby causing a setState call before mounting or after unmounting is.
|
@elasticmachine merge upstream |
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
This replaces the use of the old graphql-based `useSource` hook with the new plain JSON `useLogSource` hook. It also fixes two more problems: - A rendering problem with the source configuration loading screen and a `setState` race condition in the `useLogSource` hook. - A non-backwards-compatible change of the `/link-to/:sourceId/logs` route in elastic#61162.
* master: (24 commits) [ML] Functional tests - skip regression and classification tests [Ingest Manager] fix removing ingest pipelines from elasticsearch (elastic#75092) move tests for placeholder indices to setup (elastic#75096) [jest] temporarily extend default test timeout (elastic#75118) [cli] remove reference to removed --optimize flag (elastic#75083) skip flaky suite (elastic#75044) Adding /etc/rc.d/init.d/functions to the init script when present to … (elastic#22985) [jenkins] add pipeline for hourly security solution cypress tests (elastic#75087) [Reporting/Flaky Test] Skip test for paging list of reports (elastic#75075) remove .kbn-optimizer-cache upload (elastic#75086) skip flaky suite (elastic#74814) Actions add proxy support (elastic#74289) [ILM] TS conversion of Edit policy components (elastic#74747) [Resolver] simulator tests select elements directly instead of using descendant selectors. (elastic#75058) [Enterprise Search] Add Workplace Search side navigation (elastic#74894) [Security solution] Sourcerer: Kibana index pattern selector for security views (elastic#74706) [Logs UI] Remove apollo deps from log link-to routes (elastic#74502) [Maps] add map configurations to docker list (elastic#75035) [functional test][saved objects] update tests for additional copy saved objects to space (elastic#74907) Make the alerts plugin support generics (elastic#72716) ...
… routes Partially backports the following commits to 7.x: - [Logs UI] Remove apollo deps from log link-to routes (elastic#74502) This backport only includes the route fix for elastic#74916 to make it available in 7.9.x.
Summary
This replaces the use of the old graphql-based
useSourcehook with the new plain JSONuseLogSourcehook.It also fixes two more problems:
setStaterace condition in theuseLogSourcehook./link-to/:sourceId/logsroute in [Logs / Metrics UI] [NP followup] Cleanup link-to routes #61162.closes #74496
closes #74916
Implementation notes
This implements option 1 from #74496, because it is backwards-compatible and minimally invasive.