-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
fix: updating tp content from empty to value doesn't create the tp instance #139
base: master
Are you sure you want to change the base?
Conversation
Run & review this pull request in StackBlitz Codeflow. |
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.
Left a couple of comments, please add specs for this case as well 👍
if (!changes.content.previousValue && changes.content.currentValue) { | ||
this.initInstanceCreation(); |
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.
There is a situation where we might subscribe several times to the events.
Let's say I'm starting with no content, then adding content. I'm subscribed twice no?
I think the creation trigger should be uncoupled from the instance existence. If we already decided to create an instance, we should always create it here and not resubscribe to the events.
fix: updating tp content from empty to value doesn't create the tp instance)
Fixes #138
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
#138
What is the new behavior?
The solution here is to create/destroy the instance depending on the content changes as well and not just on initialization.
Does this PR introduce a breaking change?
Other information