-
Notifications
You must be signed in to change notification settings - Fork 394
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
feat(engine): Add support for isConnected on LightningElement #1711
Conversation
get isConnected(): boolean { | ||
const vm = getAssociatedVM(this); | ||
const { elm } = vm; | ||
return elm.isConnected; |
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.
isConnected
is polyfiled in @lwc/synthetic-shadow https://github.com/salesforce/lwc/blob/master/packages/@lwc/synthetic-shadow/src/faux-shadow/node.ts#L470
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.
The code looks good, but let's extract the unrelated code changes in multiple PR. This would allow reverting individual changes if needed.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
@@ -12,3 +12,5 @@ If yes, please describe the impact and migration path for existing applications. | |||
## The PR fulfills these requirements: | |||
* Have tests for the proposed changes been added? ✅ / ❌ | |||
* Have you followed [these instructions](../CONTRIBUTING.md#-commit-message-conventions) to clearly describe the issue being fixed or feature enhanced? ✅ / ❌ | |||
|
|||
## GUS Work Item |
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.
Let's split that in another PR.
const componentTag = getComponentTag(vm); | ||
assert.isFalse( | ||
isBeingConstructed(vm), | ||
`this.isConnected cannot be accessed during the construction phase of the custom` + |
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.isConnected cannot be accessed during the construction phase of the custom` + | |
`this.isConnected shouldn't be accessed during the construction phase of the custom` + |
assert.isFalse( | ||
isBeingConstructed(vm), | ||
`this.isConnected cannot be accessed during the construction phase of the custom` + | ||
` element for ${componentTag} because it is redundant. The value will always be` + |
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.
Even if the error makes sense in 99% of the case, I would like to bring more nuance to this statement. If the element is already connected to the DOM before the upgrade the isConnected
will be true in the constructor
: https://jsfiddle.net/pmdartus/fb1Lt8wa/2/
@@ -12,9 +12,3 @@ export interface Context { | |||
tplCache?: Template; | |||
[key: string]: any; | |||
} | |||
|
|||
export let currentContext: Context = {}; |
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.
Let's move the context removal to another PR.
5654752
to
2369ea7
Compare
Details
Developers can now check if an LWC element is connected to the DOM using the
isConnected
property on the component instance.Fixes #1698
Does this PR introduce breaking changes?
No, it does not introduce breaking changes.
If yes, please describe the impact and migration path for existing applications.
The PR fulfills these requirements:
GUS Work Item
W-7199523