-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reconnect visualizations when switching their types #11494
Conversation
Avoiding passing cached data to visualization when the source of data does not match visualization type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok if works, but I think the "reattachment" should be done "automatically" for every visualization, not only those where we call "reconnect" manually.
data: computed(() => | ||
parseVisualizationData(visualizationDataRegistry.getRawData(id.value)), | ||
), | ||
reconnect: () => (id.value = newId()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also automatically regenerate ID always when preprocessor (expression
or visualizationModule
) changed? I think this should be a "general" rule, not only for visualization displayed under graph.
It may be a bit tricky to avoid recursive watch
run: my idea is to make current id
ref to be a tuple of (id
, config); the current watch just watches this tuple, we add additional watch of configuration
which sets new tuple re-using or regenerating id, and reconnect
just regenerates id while keeping config. But maybe it may be even simpler.
- No longer using reconnect function to update visualization id, but instead doing it automatically - Found and mitigate a bug in Vue that caused inconsistent state when updating props on custom element component
A few changes happened since the last review:
|
|
const props = defineProps<{ | ||
params: { | ||
visualization?: string | object | ||
data?: any | ||
size: Vec2 | ||
nodeType?: string | undefined | ||
overflow?: boolean | ||
toolbarOverflow?: boolean | ||
} | ||
}>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify usages, we could destructure props (https://vuejs.org/guide/components/props#reactive-props-destructure) - I'm not sure if it works for a second level, but event const { params } = defineProps<...
should help.
663a67b
to
81a801f
Compare
Pull Request Description
Fixes #11426
Now, visualizations should no longer receive stale data props cached from previous visualizations opened on the same node. We do so by changing the visualization ID (reattaching the visualization).
This does not protect from unhandled exceptions happening inside visualizations, though, so a bigger fix will be provided separately.
switching.visualization.mp4
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.