Update injector for re-mounted components #138
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
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.
Description
Hi all, this is my first contribution here, please let me know if I missed something obvious. I wanted to open an issue but since the fix was quite straightforward I jumped ahead and opened this PR.
I noticed that with the current implementation if you override the default IDs created by rete with your own the renderer would not correctly update the injector context for custom elements. As stated here, the limitation was quite known already.
This PR partially addresses the limitation but at least allows the next-created elements to correctly get a new injector with the correct instances.
Related Issues
None.
Checklist
Additional Notes
I'm wondering if it is really correct to have as the identifier of the CustomElement the identifier of the node. Wouldn't be better to use the component class name? or something that does not change for each data payload contained in the node? In my understanding, in the current solution if your retejs instance contains N nodes then we are defining N different CustomElement even if we would probably need way lass (one of each CustomElement type defined by the developer).