-
Notifications
You must be signed in to change notification settings - Fork 17
Initialize properties in constructor for React. #332
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
base: main
Are you sure you want to change the base?
Conversation
| ); | ||
| }); | ||
|
|
||
| it('preserves x-element properties set before customElements.define', () => { |
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.
Just needed to add this to sure-up coverage again to 100% after the change.
|
@klebba — I am pretty confident that this will make the React interoperability just work, but it’s definitely more involved than I had initially thought. Attempts:
|
|
One other thought — it would be interesting to open up a PR for https://custom-elements-everywhere.com/ to add a test which checks that you can indeed set properties on un-upgraded (or pre-connection) custom elements and have those correctly set properties on post-upgrade (or post-connection). I.e., rather than trying to open up an issue against React, it’d be better to get a formal test added for an important edge-case in a custom-element lifecycle. I wonder if any other frameworks would fail that test 🤔 |
cfd8076 to
f19c9c5
Compare
| assert(el.internal.c === 3); | ||
| }); | ||
|
|
||
| it('throws when computed property is set before connection', () => { |
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 already tested in test-initialization-errors.js — just cleaning up the duplication.
| assert(typeof descriptor.set === 'function'); | ||
| }); | ||
|
|
||
| it('prevents deletion of properties after construction', () => { |
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.
Added some tests to lock down delete operator behavior. In this case — we now throw if you try and call delete on a property before initialization. After a lot of waffling, I think this is the correct behavior. The alternative is to silently let you delete which could cause silent bugs.
5920f19 to
1119d94
Compare
| }; | ||
| XElement.#hosts.set(host, hostInfo); | ||
| XElement.#upgradeOwnProperties(host); | ||
| // Define properties during construction for React 19 introspection (#331). |
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 the crux of the change. We need to define properties during construction so React (and other frameworks) can reliably detect that they should set values as properties (not attributes).
| const { valueMap } = hostInfo; | ||
| const { key } = property; | ||
| Reflect.defineProperty(host, key, { | ||
| configurable: false, |
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.
Note that we set the real accessor descriptor during construction. We have to add a small internal fork… but I think it’s worth it. Importantly, we set this once and only once as a non-configurable accessor descriptor. This is why it will fail if you try to delete one of these properties post-construction, even pre-initialization.
1119d94 to
2d74d0e
Compare
| enumerable: true, | ||
| get() { | ||
| const { initialized } = hostInfo; | ||
| if (initialized) { |
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.
Again, we have an internal fork here… but I think this is perhaps the best we can do. The internal logic is slightly different for setting / getting properties before initialization. Importantly, we just dump values to a dumb value store pre-initialization and then do all the validation / initialization only after connection.
I went down a rabbit hole investigating whether we ought to try and fully support properties post-construction / pre-initialization. However, things get weird pretty fast… you get into this strange middle-ground where something work perfectly and then others… only somewhat. Most of that stems from rules around conformance for custom elements during construction. In particular, some of these rules exist because an element can be constructed before its attributes are fully parsed… this means you would have to deal with this middle-ground where you are trying to compute properties, sync programmatic attributes, reflect programmatic attributes, and render… but you might be missing attributes which are yet-to-be-parsed in the markup.
Anyways… this is what I netted out with.
2d74d0e to
5330fa8
Compare
| const { value, found } = XElement.#getPreUpgradePropertyValue(host, property); | ||
| XElement.#initializeProperty(host, property); | ||
| if (found) { | ||
| host[property.key] = property.default(host, property.initial(value)); |
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.
There were a couple options here:
- Introduce a new flag like
initializing(versusinitialized) which could help us internally “do the right thing” when we set a value on the host… - Use specific, internal methods during initialization because we accept that we are in a strange half-way state and it doesn’t really make sense to be using the full functionality just yet.
I went with (2) because the code felt cleaner overall. Additionally, by doing this, I was actually able to _remove _ an internal fork within #invalidateProperty 🤙
5330fa8 to
37bb0af
Compare
React doesn’t provide a way for developers to signal that they want to set a property (versus an attribute) in JSX — instead it performs some magic to _guess_ what was meant. This _guess_ is based on whether the expression `key in element` is `true`. As a best-practice, XElement is _lazy_ about initialization and waits until it is _actually connected to the DOM_ before initializing all properties. But… that means React will always check for setters _before_ they exist! This change set adds our accessor descriptors during construction, but adds an internal flag to prevent certain behavior until the component is fully initialized during connection. Closes #331.
37bb0af to
a443249
Compare
| let found = false; | ||
| if (!internal) { | ||
| // Only look for public (i.e., non-internal) properties. | ||
| if (Reflect.has(host, key)) { |
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 Reflect.has basically just got moved up to construction-time when we setup our initial valueMap.
| return { value, found }; | ||
| } | ||
|
|
||
| static #initializeProperty(host, property) { |
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 more-or-less just gets renamed to #defineProperty. Could have called it #constructProperty… I waffled a bit there… But, #initializeProperty is no longer the right language.
|
|
||
| static #invalidateProperty(host, property) { | ||
| const { initialized, invalidProperties, computeMap } = XElement.#hosts.get(host); | ||
| if (initialized) { |
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 was a big win — because we moved our forks higher up the call stack… we no longer need to check for initialized so deep in the call stack here. I never liked that we had to do this.
| return property.compute?.(host) ?? valueMap.get(property); | ||
| } | ||
|
|
||
| static #validatePropertyMutable(host, property) { |
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.
Had to factor this out so that we can call it surgically during initialization.
React doesn’t provide a way for developers to signal that they want to set a property (versus an attribute) in JSX — instead it performs some magic to guess what was meant. This guess is based on whether the expression
key in elementistrue.As a best-practice, XElement is lazy about initialization and waits until it is actually connected to the DOM before initializing all properties. But… that means React will always check for setters before they exist!
This change set adds our accessor descriptors during construction, but adds an internal flag to prevent certain behavior until the component is fully initialized during connection.
Closes #331.
TODO:
deleteought to work on elements… we have no prior tests on support for likedelete el.fooin the pre-upgraded, pre-connected, and post-connected states. Might be worth considering what the “right” thing is here.deleteoperator behavior and lock down expectations.