feat(ui): Only send new exported objects #129
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mofojed
force-pushed
the
exported-objects-handling
branch
from
c8cdf4d
to
804c5c3
Compare
|
@niloc132 I want to ensure this approach is valid given our discussions about exported objects. Though I think one thing I'll need to change is to export an object multiple times if it is used multiple times in the document (since we're only allowing one |
mofojed
force-pushed
the
exported-objects-handling
branch
from
804c5c3
to
e3464d7
Compare
mattrunyon
requested changes
Dec 11, 2023
plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py
Outdated
Show resolved
Hide resolved
Comment on lines
+48
to
+51
| const exportedObjectMap = useRef<Map<number, WidgetExportedObject>>( | ||
| new Map() | ||
| ); | ||
| const exportedObjectCount = useRef(0); |
There was a problem hiding this comment.
In the deephaven express version of this, we added an array of new object IDs to the message from the server to align with the array received instead of doing the same incrementing on the client. We weren't sure if it was fully safe to just increment on the client. If messages can somehow arrive out of order, then the client is now in a bad state and doesn't know it. We also send an array of object IDs that can be closed to the client
This is fine if we can guarantee message order from the server
There was a problem hiding this comment.
Messages will arrive in the order they are sent.
- Client retains a reference to previously exported objects if they're the same object - Client builds the map of IDs to exported objects when each new documentUpdated event is received - A strong reference is retained to all the exported objects in the current render so they do not go out of scope unexpectedly - A weak reference is stored from the ID to the exported object - Not tested at all
- Added some missing types in Element and RenderedNode - Added unit tests to verify that objects/callables stay valid after updates
- It cleans up dead objects, which we only want to do when parsing an actual documentUpdated request rather than decoding any response (such as a response for a callback) - Cleaned up how ElementMessageStream encodes the payload and sends it back.
- Based on latest review cleanup
mofojed
force-pushed
the
exported-objects-handling
branch
from
e3464d7
to
d120b83
Compare
- NodeEncoder now has a encode_node method, and throws if you try and call `.encode()` directly - Fixed up tests
mattrunyon
requested changes
Dec 12, 2023
plugins/ui/src/js/src/ObjectView.tsx
Outdated
| @@ -13,7 +13,11 @@ function ObjectView(props: ObjectViewProps) { | |||
| const { object } = props; | |||
| log.info('Object is', object); | |||
|
|
|||
| const fetch = useCallback(() => object.fetch() as Promise<Widget>, [object]); | |||
| const fetch = useCallback(async () => { | |||
| // We reexport the object in case this object is used in multiplate places or closed/opened multiple times | |||
There was a problem hiding this comment.
Suggested change
| // We reexport the object in case this object is used in multiplate places or closed/opened multiple times | |
| // We re-export the object in case this object is used in multiple places or closed/opened multiple times |
Comment on lines
+25
to
+26
| const reexportedTable = await element.props.table.reexport(); | ||
| const newTable = (await reexportedTable.fetch()) as Table; |
There was a problem hiding this comment.
Should we add this to the main table plugin too?
There was a problem hiding this comment.
We shouldn't need it there - see that fetch is wrapped in ObjectView to do the re-export already.
mattrunyon
approved these changes
Dec 13, 2023
mofojed
added a commit
that referenced
this pull request
Dec 20, 2023
- When decoding the JSON document, convert the element nodes right to their components then - Eliminates wrapper components like `ElementView`, which was causing some issues when used within components that expected children of a certain type (such as Tabs/TabPanels/TabList from spectrum) - Wraps exported objects in the `ObjectView` only when they are rendered as children of an element - Still checks for mixed panel/non-panels at root, and implicitly wraps root if no panels - Wrap children elements of spectrum components that are just `string` type to a `Text` element - Fixes up padding issues within Spectrum components - Fix styling on table to `display: contents` - Now it sizes correctly by default when used within tabs - Use a context (ReactPanelManagerContext) to handle the opening/closing of react panels within a document - Concept can be extended for opening/closing dashboards as well - Add an example for `Tabs`/`ui.tabs` - Looked into automatically mapping `ui.item`, but was annoying with setting the keys, not bothering to deviate right now - Handle exported object lifecycle more correctly - Now the `WidgetHandler` handles closing all the exported objects - This will conflict with #129, and needs further discussion on how best to handle this, as some objects can't be copied (only `Table` can be) and we may have the same object twice in a document - Fixes #127
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: JSON protocol now keeps the IDs of objects and callables stable between re-renders, and only exports new objects with each documentUpdated event. The client must track the old objects independently.