Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(lightning-element): expose hostElement property #4259

Merged
merged 7 commits into from
Jun 14, 2024
Merged

feat(lightning-element): expose hostElement property #4259

merged 7 commits into from
Jun 14, 2024

Conversation

rochejul
Copy link
Contributor

@rochejul rochejul commented Jun 5, 2024 •
edited by nolanlawson
Loading

Details

Fixes #4219

Related to issue 4219 for the following feature:

As a component author, I want to be able to access the real HTMLElement of my LightningElement from within one of its callbacks (e.g. renderedCallback), consistently in both shadow DOM and light DOM.

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.

GUS work item

@rochejul rochejul requested a review from a team as a code owner June 5, 2024 09:53
@rochejul rochejul mentioned this pull request Jun 5, 2024
Closed
@wjhsf wjhsf mentioned this pull request Jun 5, 2024
Closed
nolanlawson
nolanlawson reviewed Jun 5, 2024
View reviewed changes
packages/@lwc/engine-core/src/framework/base-lightning-element.ts Outdated
const isRenderLight = vm.renderMode === RenderMode.Light;

if (isRenderLight) {
return vm.elm;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure you can just return vm.elm everywhere, without regard to light/shadow mode.

nolanlawson
nolanlawson reviewed Jun 5, 2024
View reviewed changes
Copy link
Collaborator

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a bunch for this PR! 😄

This LGTM, although we want to do some internal bikeshedding first before we decide on the name. It may change from elementSelf before it gets merged.

@nolanlawson nolanlawson mentioned this pull request Jun 6, 2024
Closed
nolanlawson
nolanlawson reviewed Jun 6, 2024
View reviewed changes
packages/@lwc/engine-core/src/framework/base-lightning-element.ts Outdated
@@ -542,6 +543,11 @@ function warnIfInvokedDuringConstruction(vm: VM, methodOrPropName: string) {
return vm.shadowRoot;
},

get elementSelf(): Node | undefined {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
get elementSelf(): Node | undefined {
get elementSelf(): Element {

I don't see how this could possibly be undefined or a Node. The vm.elm type may not be strict enough, so you might have to cast it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also put a sanity check in here:

if (process.env.NODE_ENV !== 'production') {
  assert.isTrue(!isUndefined(vm.elm) && vm.elm instanceof Element, `this.elementSelf should be an Element, found: ${vm.elm}`)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: just check vm.elm instanceof Element because undefined instanceof Element works just fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL! Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback

nolanlawson
nolanlawson reviewed Jun 6, 2024
View reviewed changes
packages/@lwc/engine-core/src/framework/base-lightning-element.ts Outdated
@@ -194,6 +194,7 @@ export interface LightningElement extends HTMLElementTheGoodParts, AccessibleEle
constructor: LightningElementConstructor;
template: ShadowRoot | null;
refs: RefNodes | undefined;
elementSelf: Node | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elementSelf: Node | undefined;
elementSelf: Element;

See below.

@rochejul rochejul force-pushed the feat/4219-access-real-html-element-in-consistently-way branch from c5faf8b to 6b7ece5 Compare June 10, 2024 07:02
@nolanlawson nolanlawson mentioned this pull request Jun 10, 2024
Closed
nolanlawson
nolanlawson reviewed Jun 10, 2024
View reviewed changes
packages/@lwc/engine-core/src/framework/base-lightning-element.ts Outdated

if (process.env.NODE_ENV !== 'production') {
if (isFalse(vm.elm instanceof Element)) {
logError(`this.elementSelf should be an Element, found: ${vm.elm}`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should do assert.false here instead.

Historically we do try to avoid throwing dev-only errors, but in this case I don't think it applies, because this is an invariant in the framework itself. If we are returning something other than an Element here, then something has gone horribly wrong and we need to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, let me have a look on it

@rochejul rochejul force-pushed the feat/4219-access-real-html-element-in-consistently-way branch from 2fe2853 to 98d6d13 Compare June 12, 2024 11:11
@nolanlawson
Copy link
Collaborator

Based on #4275 it looks like we are going to go with hostElement instead of elementSelf. I'll make that change and then merge this – I don't think we need API versioning since hostElement should be pretty unique.

@nolanlawson nolanlawson mentioned this pull request Jun 14, 2024
Closed
@nolanlawson nolanlawson mentioned this pull request Jun 14, 2024
Closed
@nolanlawson nolanlawson changed the title feat(lightning-element): expose elementSelf property feat(lightning-element): expose hostElement property Jun 14, 2024
@nolanlawson
Copy link
Collaborator

I also went ahead and made this.hostElement throw an error on the server side ^

I don't think this is really needed on the server, so we should proactively block it.

@nolanlawson
Copy link
Collaborator

/nucleus test

@nolanlawson nolanlawson mentioned this pull request Jun 14, 2024
Closed
@nolanlawson
Copy link
Collaborator

/nucleus ignore -- reason 'downstream flaky test'

@salesforce-nucleus Salesforce Nucleus
Copy link
Contributor

❌ Error Parsing Command

Missing required argument: reason

/nucleus ignore

Ignore test failures

Options:
  --help        Show help                                              [boolean]
  --reason, -m  A descriptive reason for ignoring the test results
                                                             [string] [required]

Examples:
  /nucleus ignore --reason "Downstream dependency X has a flapping test"

Missing required argument: reason

@nolanlawson
Copy link
Collaborator

/nucleus ignore --reason 'downstream flaky test'

@nolanlawson nolanlawson merged commit 8b3f436 into salesforce:master Jun 14, 2024
13 of 16 checks passed
@nolanlawson
Copy link
Collaborator

Thank you!

@rochejul
Copy link
Contributor Author

Thanks you so much @nolanlawson

@nolanlawson nolanlawson mentioned this pull request Jun 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjhsf wjhsf wjhsf left review comments

@nolanlawson nolanlawson nolanlawson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Be homogenous to traverse the parent DOM between shadow dom and lwc:render-mode="light" mode
3 participants
@rochejul @nolanlawson @wjhsf