refactor(compass-schema): replace reflux with redux COMPASS-8797 #6656
Conversation
packages/compass-schema/src/components/coordinates-minichart/coordinates-minichart.jsx
Outdated
Show resolved
Hide resolved
| _trackSchemaAnalyzed(analysisTime, query); | ||
|
|
||
| dispatch(onSchemaSampled()); | ||
| } catch (err: any) { |
There was a problem hiding this comment.
I haven't looked at what exactly analyzeSchema is doing with the signal, but usually aborting a signal results in promise rejection, so we probably want to check for that here so that stop and error are not dispatched at the same time? I think the usual pattern for this we use is instead of subscribing to signal, you'd check on error that it's caused by signal being aborted and then dispatch accordingly
There was a problem hiding this comment.
in this case analyzeSchema returns null when it's cancelled:
the null schema is then handled by AnalysisFinishedAction. Hmm.. and I added a superfluous AnalysisCancelledAction, let me clean that up
paula-stacho
force-pushed
the
COMPASS-8797
branch
from
b825683 to
c61a63b
Compare
| geo_data: schema ? schemaContainsGeoData(schema) : false, | ||
| analysis_time_ms: analysisTime, | ||
| }); | ||
| track('Schema Analyzed', trackEvent, connectionInfoRef.current); |
There was a problem hiding this comment.
Also I understand that it's something that's already working like this, but it might make sense to ask product if they expect that this is tracked when analysis is canceled
There was a problem hiding this comment.
Thank you for pointing this out! Waiting for confirmation, but we'll likely split this as part of https://jira.mongodb.org/browse/COMPASS-8818
|
|
||
| export const stopAnalysis = (): SchemaThunkAction<void> => { | ||
| return (dispatch, getState, { abortControllerRef }) => { | ||
| if (!abortControllerRef) return; |
There was a problem hiding this comment.
Based on types and the plugin setup code this value is always there, so this check is not doing anything, what's the purpose?
There was a problem hiding this comment.
ah, that was supposed to check for .current, thanks for spotting!
Description
Main changes:
setStatecalls replaced with a reducerSide changes:
Math.random)Note: still figuring out how to test the geoLayers
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes