-
Notifications
You must be signed in to change notification settings - Fork 0
Add batched propschange event
#129
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
Merged
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
6468c6e
Add batched `propschange` event
LeaVerou defe485
Extract PropChangeEvent / PropsChangeEvent prop typedefs
LeaVerou df9be12
Move propchange alias dispatch into events/propchange plugin
LeaVerou a42fdbc
Inline #firePropChangeEvent into propChanged
LeaVerou 0510f89
Rework ElementProps burst handling around an ordered event log
LeaVerou c1dab59
Attach on*/alias listeners early so no catch-up is needed
LeaVerou 32b0d18
Extract propschange into a separate plugin
LeaVerou c49d126
Restructure props/ to match events/ meta-plugin pattern
LeaVerou b12c1f7
Apply suggestions from code review
LeaVerou 0becf80
Fix stale attribute fields leaking across coalesced propchange events
LeaVerou 8f5c234
Prettier
LeaVerou b719f4b
Use `this.source`
LeaVerou 58e7d30
Split prop input from derived value in ElementProp
LeaVerou 444cb32
Support custom sources better
LeaVerou 92f3e0f
Update src/plugins/props/util/PropChangeEvent.js
LeaVerou ca48136
Fire mount event for every prop, not just plain-default ones
LeaVerou 457750e
Add propschange regression tests for round-trip and re-entrant writes
DmitrySharabin caf6f2c
Guard spec.parse(), event cloning, and remove applyTo() (#132)
DmitrySharabin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| import Props from "./util/Props.js"; | ||
| import Prop from "./util/Prop.js"; | ||
| import ElementProps from "./util/ElementProps.js"; | ||
| import ElementProp from "./util/ElementProp.js"; | ||
| import { symbols } from "xtensible"; | ||
| import { defineOwnProperty, getSuperMethod } from "xtensible/util"; | ||
| import { defineLazyProperty } from "../../util/lazy.js"; | ||
| import PropType from "./util/PropType.js"; | ||
| import "./types/index.js"; | ||
|
|
||
| export const { props } = symbols.known; | ||
|
|
||
| export { PropType, Props, Prop, ElementProps, ElementProp }; | ||
|
|
||
| const hooks = { | ||
| setup () { | ||
| if (Object.hasOwn(this, "props")) { | ||
| this.defineProps(); | ||
| } | ||
| }, | ||
|
|
||
| constructor () { | ||
| if (!this.constructor[props]) { | ||
| return; | ||
| } | ||
|
|
||
| if (this.propChangedCallback) { | ||
| this.addEventListener("propchange", this.propChangedCallback); | ||
| } | ||
| }, | ||
|
|
||
| connected () { | ||
| this.props.paused = false; | ||
| }, | ||
|
|
||
| disconnected () { | ||
| this.props.paused = true; | ||
| }, | ||
|
|
||
| "attribute-changed" ({ name, oldValue }) { | ||
| this.props.attributeChanged(name, oldValue); | ||
| }, | ||
| }; | ||
|
|
||
| const provides = { | ||
| props: undefined, // see below | ||
|
|
||
| // Must be on the prototype before customElements.define runs — spec reads observedAttributes only if ACB is non-null. | ||
| attributeChangedCallback (name, oldValue, value) { | ||
| // super.attributeChangedCallback() | ||
| getSuperMethod(this, provides.attributeChangedCallback)?.call(this, name, oldValue, value); | ||
|
|
||
| this.$hook("attribute-changed", { name, oldValue, value }); | ||
| }, | ||
|
|
||
| constructor: { | ||
| defineProps (def = this.props) { | ||
| if (def instanceof Props && def.Class === this) { | ||
| // Already defined | ||
| return null; | ||
| } | ||
|
|
||
| let env = { props: def }; | ||
| this.$hook("define-props", env); | ||
|
|
||
| this[props].add(env.props); | ||
| }, | ||
|
|
||
| get observedAttributes () { | ||
| // FIXME how to combine with existing observedAttributes? | ||
| let attributes = [...this[props].allValues()] | ||
| .map(prop => prop.reflect.from) | ||
| .filter(Boolean); | ||
| return [...new Set(attributes)]; | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| /** | ||
| * Per-element collection of {@link ElementProp} wrappers, materialized on | ||
| * first access. The {@link ElementProps} constructor self-installs as the | ||
| * element's own `props` data property, shadowing this accessor from then on. | ||
| */ | ||
| defineLazyProperty(provides, "props", { | ||
| get () { | ||
| if (this.constructor.props) { | ||
| return new ElementProps(this); | ||
| } | ||
| }, | ||
| configurable: true, | ||
| writable: true, | ||
| }); | ||
|
|
||
| defineOwnProperty(provides.constructor, props, function () { | ||
| return new Props(this, this.props); | ||
| }); | ||
|
|
||
| export default { hooks, provides }; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Should mount-drain
propchanges be re-dispatched as semantic alias events?This re-broadcasts every
propchange— including the mount-time default fire — as its alias (spacechange,valuechange, …). So a consumer listening for the alias gets a "change" for a value that never changed (the initial default).Real case that bit me: a parent listens to a nested child's
spacechangeand writes the child's value back into its own reflected prop:At mount the child's computed
selectedSpacefires with its default (a98rgb), this listener re-fires it asspacechange, and the parent's authoredvalue="oklab.a"gets overwritten (and reflected back to the attribute, destroying the markup). Because it races the parent's own value-sync, it's intermittent.The catch is that for a computed aliased prop you can't filter it by
source: both the mount fire and a genuine post-mount cascade arrive withsource: undefined(ElementProp.update()clears it) — onlyoldValue === undefinedseparates them. AndPropChangeEvent.applyTokeys offsource, so it would also refuse to mirror real computed-prop changes.So: should aliases fire during the mount drain for initial/default values at all — and if they do, what's the intended signal to tell "initial" from "changed"? (Pre-#106 these carried
source: "initial", which at least gave consumers a marker to filter on.)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 the whole idea of
applyTo()is fundamentally broken TBH. And for most callsites in color-elements replacing it would be a one line fix, I think. So I wonder if we should just get rid of it…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.
With the adoption of
updated({change}), none of the color elements use it. So, yes, we can get rid of it.