Clone the root in Document::from_data_model_instance #110
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
filiptibell
approved these changes
Sep 25, 2023
There was a problem hiding this comment.
Thank you for getting this fixed!
I think this may also be a problem for
Document::from_instance_array
Hmm, I wonder if multi-root models with refs across those roots is supported by roblox (probably is?), and if it is, have we seen anyone use that? Either way we should probably support that too, yeah.
Tests look fine also. 👍
|
Superseded by #117 |
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.
Closes #109 (this is squarely my bug :D).
This PR alters
Document::from_data_model_instanceto clone the root directly instead of cloning each child, thereby preserving Ref properties that point across children. rbx-dom can probably better support this use case, but I'll need to investigate what the best solution isI think this may also be a problem for
Document::from_instance_array... we might be able to do something similar to this PR and create a dummy instance, clone_within the entire instance array as children, then do the transfer+destroy dance?Sorry about the test file diff - I opted to use
doblocks to prevent variable shadowing. I can just use different variable names if preferred