-
Notifications
You must be signed in to change notification settings - Fork 815
feat(runtime): watch native HTML attributes #4760
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
Changes from all commits
cc4b340
8a9c4f6
f4d4b29
361c262
5ebaf76
b1ba2db
6248025
39ec30d
b2440d6
d12ac84
eb73411
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1386,6 +1386,9 @@ export type ComponentRuntimeMetaCompact = [ | |
|
||
/** listeners */ | ||
ComponentRuntimeHostListener[]?, | ||
|
||
/** watchers */ | ||
ComponentConstructorWatchers?, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed to add this so we can have a map of the watchers available when bootstrapping the lazy-build before the module is loaded. Otherwise, the class instance that gets registered with the custom element registry won't include any of the watched native attributes in the |
||
]; | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,7 +85,7 @@ export const proxyComponent = ( | |
if (BUILD.observeAttribute && (!BUILD.lazyLoad || flags & PROXY_FLAGS.isElementConstructor)) { | ||
const attrNameToPropName = new Map(); | ||
|
||
prototype.attributeChangedCallback = function (attrName: string, _oldValue: string, newValue: string) { | ||
prototype.attributeChangedCallback = function (attrName: string, oldValue: string, newValue: string) { | ||
plt.jmp(() => { | ||
const propName = attrNameToPropName.get(attrName); | ||
|
||
|
@@ -133,25 +133,60 @@ export const proxyComponent = ( | |
// if the propName exists on the prototype of `Cstr`, this update may be a result of Stencil using native | ||
// APIs to reflect props as attributes. Calls to `setAttribute(someElement, propName)` will result in | ||
// `propName` to be converted to a `DOMString`, which may not be what we want for other primitive props. | ||
return; | ||
} else if (propName == null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of the stuff in this block comes from the existing |
||
// At this point we should know this is not a "member", so we can treat it like watching an attribute | ||
// on a vanilla web component | ||
const hostRef = getHostRef(this); | ||
const flags = hostRef?.$flags$; | ||
|
||
// We only want to trigger the callback(s) if: | ||
// 1. The instance is ready | ||
// 2. The watchers are ready | ||
// 3. The value has changed | ||
if ( | ||
!(flags & HOST_FLAGS.isConstructingInstance) && | ||
flags & HOST_FLAGS.isWatchReady && | ||
newValue !== oldValue | ||
) { | ||
const elm = BUILD.lazyLoad ? hostRef.$hostElement$ : this; | ||
const instance = BUILD.lazyLoad ? hostRef.$lazyInstance$ : (elm as any); | ||
const entry = cmpMeta.$watchers$[attrName]; | ||
entry?.forEach((callbackName) => { | ||
if (instance[callbackName] != null) { | ||
instance[callbackName].call(instance, newValue, oldValue, attrName); | ||
} | ||
}); | ||
} | ||
|
||
return; | ||
} | ||
|
||
this[propName] = newValue === null && typeof this[propName] === 'boolean' ? false : newValue; | ||
}); | ||
}; | ||
|
||
// create an array of attributes to observe | ||
// and also create a map of html attribute name to js property name | ||
Cstr.observedAttributes = members | ||
.filter(([_, m]) => m[0] & MEMBER_FLAGS.HasAttribute) // filter to only keep props that should match attributes | ||
.map(([propName, m]) => { | ||
const attrName = m[1] || propName; | ||
attrNameToPropName.set(attrName, propName); | ||
if (BUILD.reflect && m[0] & MEMBER_FLAGS.ReflectAttr) { | ||
cmpMeta.$attrsToReflect$.push([propName, attrName]); | ||
} | ||
return attrName; | ||
}); | ||
// Create an array of attributes to observe | ||
// This list in comprised of all strings used within a `@Watch()` decorator | ||
// on a component as well as any Stencil-specific "members" (`@Prop()`s and `@State()`s). | ||
// As such, there is no way to guarantee type-safety here that a user hasn't entered | ||
// an invalid attribute. | ||
Cstr.observedAttributes = Array.from( | ||
new Set([ | ||
...Object.keys(cmpMeta.$watchers$ ?? {}), | ||
...members | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't include all "members" here (regardless if they have an associate watcher), then they components will not set their values and update appropriately. |
||
.filter(([_, m]) => m[0] & MEMBER_FLAGS.HasAttribute) | ||
.map(([propName, m]) => { | ||
const attrName = m[1] || propName; | ||
attrNameToPropName.set(attrName, propName); | ||
if (BUILD.reflect && m[0] & MEMBER_FLAGS.ReflectAttr) { | ||
cmpMeta.$attrsToReflect$.push([propName, attrName]); | ||
} | ||
|
||
return attrName; | ||
}), | ||
]), | ||
); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { Component, Element, h, State, Watch } from '@stencil/core'; | ||
|
||
@Component({ | ||
tag: 'watch-native-attributes', | ||
}) | ||
export class WatchNativeAttributes { | ||
@Element() el!: HTMLElement; | ||
|
||
@State() callbackTriggered = false; | ||
|
||
@Watch('aria-label') | ||
onAriaLabelChange() { | ||
this.callbackTriggered = true; | ||
} | ||
|
||
render() { | ||
return [ | ||
<p>Label: {this.el.getAttribute('aria-label')}</p>, | ||
<p>Callback triggered: {`${this.callbackTriggered}`}</p>, | ||
]; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
<!doctype html> | ||
<meta charset="utf8" /> | ||
<script src="./build/testapp.esm.js" type="module"></script> | ||
<script src="./build/testapp.js" nomodule></script> | ||
|
||
<watch-native-attributes aria-label="myStartingLabel"></watch-native-attributes> |
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.
We do lose a compile-time check with this change. Only way around it would be to maintain a list of all the valid HTML attributes to check against which would be a nightmare.
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.
I think if we're opening it up to any attribute then this makes sense, it's the developers' responsibility to make sure that the string used is a valid one
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.
Agreed - we run into this with mock doc & checking for reserved keys today, and don't have a great mechanism for checking if we're missing a standardized HTML attribute.
RE This comment in the PR summary:
Is the Framework's request based on supporting
aria-*
attributes only? Or is there more they're looking to support? I'm wondering in there's a compromise to be had here if we opened this up to just a limited subset of attributes (in this example, those prefixed witharia-*
).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 only examples they've given have been
aria
, but not sure if they would use others as well. What would be the benefit of restricting this though? And, inversely, what is the risk of allowing users to watch any attribute?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.
I think (maybe) the benefit here is that if we limited this to
aria-*
, we'd have a more finite list (and perhaps stable) list of attributes to check against, which would influence my opinion on some of the comments here.I don't think there's a particular risk I'm concerned about ATM - I'm just looking for some additional details regarding the context of their request. Maybe we can add/attach that to this PR summary or a JIRA ticket?
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.
In Framework we maintain a list of attributes to inherit here: https://github.com/ionic-team/ionic-framework/blob/f82709595d5031a759e35b11dbcccb0ea5870ce0/core/src/utils/helpers.ts#L118-L175
But our reasoning is to not inherit all attributes that exist on the host. I'm not really sure why you'd restrict what attribute a developer can watch a value change on. That's based on the implementation details of the component, not a compiler constraint.
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.
@sean-perkins That's great to know!
Getting that context is what I'm really after here to both:
In that case, I think this code/logic fine as is 👍