feat: unified actions selectors #2749
Conversation
| return derived( | ||
| [ctx.runtime, ctx.metricsViewName], | ||
| ([runtime, metricViewName], set) => { | ||
| return createRuntimeServiceGetCatalogEntry( | ||
| runtime.instanceId, | ||
| metricViewName, | ||
| { | ||
| query: { | ||
| select: (data) => | ||
| selector | ||
| ? selector(data?.entry?.metricsView) | ||
| : data?.entry?.metricsView, | ||
| queryClient: ctx.queryClient, | ||
| }, | ||
| } | ||
| ).subscribe(set); | ||
| } | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Should we use the v5 reactivity pattern and put the derived store within the createQuery (or createOrvalGeneratedQuery) call?
There was a problem hiding this comment.
Really interesting idea that I hadn't considered. Let's discuss as a team, I see pros and cons:
- pro: will make moving to sq5 easier later
- cons: will require more code changes as we refactor here. Not clear yet how exactly we want to use sq5 API so we may be guessing at this point
There was a problem hiding this comment.
Would we need to make tweaks to orval generator? Does it even allow us to do this?
There was a problem hiding this comment.
Good point @AdityaHegde , I don't think we could touch the orval generated query.
What we could do is make our options into these wrapper methods stores, so at least it matches future APIs.
But IMO its overkill for now. We should wait until sq5 is stable before we try to do any migration in that direction, with the exception of the query client thing because we can't get around that.
| (meta) => !!meta?.timeDimension | ||
| ) as CreateQueryResult<boolean>; | ||
|
|
||
| export const getFilterSearchList = ( |
There was a problem hiding this comment.
Great! I was looking to do something similar in my time control refactor (The derived store would only depend on dashboardStore and the new time range data store). Perhaps makes senes to add more such extractions in future PRs.
…o feat/unified-actions-selectors
| absenceCallback?: () => MetricsExplorerEntity | ||
| ) => { | ||
| const name = get(dashboardStore).name; | ||
| updateMetricsExplorerByName(name, callback, absenceCallback); |
There was a problem hiding this comment.
This redirect to updateMetricsExplorerByName feels unnecessary. Perhaps add a TODO to move the body of updateMetricsExplorerByName here once we refactor everything to use updateDashboard.
There was a problem hiding this comment.
Yes I agree, I will leave a TODO here for now.
The reason I didn't outright do it yet is that it requires access to the update method from the MetricsExplorerStore, and that method is currently not publicly exposed outside of the singleton file where MetricsExplorerStore is created.
* feat: init context sharing * example * fix: lint issue * cleanup * ex refactor wip * wip refactor * cleanup * type * fix: fork sq * cleanup * cleanup * move value check to enabled prop * remove business model * remove business model * fix merge * fix: type error * fix: types * feat: consolidate action code * fix: remove unused * fix: added comment * fix: optional search results
Checklist
Summary
Issue addressed:
Details:
Steps to Verify