-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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(custom-element): properly handle custom element remove then insert again #12413
base: main
Are you sure you want to change the base?
Conversation
…ove element and append it back after fully unmounted (vuejs#12412)
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-ssr
@vue/compiler-sfc
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
} else if (__DEV__) { | ||
warn(`Exposed property "${key}" already exists on custom element.`) | ||
} else { | ||
delete exposed[key] // delete it from exposed in case of deleting wrong exposed key when disconnected |
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.
Could you provide more info on why need this line?
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.
This is to remove invalid exposed key.
Let's say we have an element with prop foo
, and we accidentally expose an object {foo: 1}
, it will be ignored as foo
is already on this element. In disconnectedCallback, we'll delete exposed properties according to exposed
, but deleting foo
can cause an error as it's non-configurable prop.
Therefore, we should remove that key ahead
Thank you for your contribution. I have some suggestions and questions.
I think the reason for the error here is that // in disconnectedCallback
this._parent = undefined
this._resolved = false
The fix to this part are generally correct. However, I don't understand why this line needs to be added.
I think this change is no longer necessary if reset |
@edison1105 Thanks for your suggestions! It's a good fix for current issues. I will check more on this and update issue and commit this night |
Actually setting |
re-mount should also re-apply the style, right ? |
…f again instead of only re-mount
Oh, I forgot to run e2e tests in my local, my bad. I can see it's from |
The reason |
While fixing #12453, I realized I made a mistake and my previous review was incorrect. Your initial submission might have been correct. if (this._resolved) {
this._setParent()
this._update()
} We should reuse the already resolved |
Updated, thanks for the info |
close #12412
changes:- extract observer code- observe again and re-mount if it's resolved and no instance, new parent will be set on new mount- exposed properties should be configurable, and we should delete them if element is fully unmounted, so that we can expose again in new mountchanges:- extract unmount code, delete exposed properties when unmounting, clear parent and set resolved to false- unmount if parent changed when connected- delete invalid exposed key from exposed when mountingchanges: