-
-
Notifications
You must be signed in to change notification settings - Fork 113
Conversation
✅ Deploy Preview for vapor-template-explorer ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for vapor-repl ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Size ReportBundles
Usages
|
dccd6e3
to
6c99c7a
Compare
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.
LGTM for the rest part
: slot.fn, | ||
) | ||
dynamicSlotKeys[slot.name] = true | ||
renderEffect(() => { |
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.
Why change here? slots
can be accessed by public API useSlots
. So we need to update it before effects from user-land. Just like attrs
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.
When making the modification, I did not take this situation into account, but I have now corrected it.
Maybe we can add a unit test for this feature later?
related: #154 (comment)