Alternative value subscription implementation #310
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
chrfalch
reviewed
Mar 25, 2022
There was a problem hiding this comment.
I think this looks very promising, like the idea of implementing the value registration in a separate class again - since it removes this from the container which becomes much cleaner.
I only have a question about unsubscribing before adding values when setting properties - wouldn't we need to unsubscribe first? (see comment in code).
package/src/renderer/Host.ts
Outdated
| } | ||
|
|
||
| abstract draw(ctx: DrawingContext): void | DeclarationResult; | ||
|
|
||
| set props(props: AnimatedProps<P>) { | ||
| this.depMgr.addValues(props); |
There was a problem hiding this comment.
Wondering if we need to unsubscribe first before subscribing? If we get property updates?
|
@chrfalch Thanks for the review, I implemented the comments, it is now identical to the previous implementation but much simpler. |
Now the SkiaDrawView on iOS will listen to the RCTBridgeWillInvalidateModulesNotification event on the notification center to tear down and unregister views on hot reload instead of after views are removed.
…ich can cause a crash
…l children in the removeNode method. The removeNode method is only called from the reconciler on the topmost node that is removed, and the documentation says that if we want to release any resources used by children of the removed node we need to do so ourselves - so this has been added.
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.
This is an alternative implementation of #264
fixes #230
fixes #261