Add custom element property support behind a flag #22184
Conversation
|
Hi @josepharhar! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
@sebmarkbage please take a look! |
|
Comparing: c7917fe...4bd3b44 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
packages/shared/ReactFeatureFlags.js
Outdated
| // and client rendering, mostly to allow JSX attributes to apply to the custom | ||
| // element's object properties instead of only HTML attributes. | ||
| // https://github.com/facebook/react/issues/11347 | ||
| export const enableCustomElementPropertySupport = false; |
There was a problem hiding this comment.
It's ok to set this to __EXPERIMENTAL__ and it'll be on in the experimental builds but the the "next" stable release channel which 18 is on.
That way it'll have some test coverage in CI on and you get a build to experiment with.
There was a problem hiding this comment.
Awesome!!
Should I set it to __EXPERIMENTAL__ in all of the separate feature flag files I had to add this flag to?
| if (enableCustomElementPropertySupport && propKey === 'className') { | ||
| // className gets rendered as class on the client, so it should be | ||
| // rendered as class on the server. | ||
| propKey = 'class'; |
There was a problem hiding this comment.
What about htmlFor? Is that needed here?
There was a problem hiding this comment.
className is special because all elements have a builtin className property which triggers the 'className' in node check when rendering on the client - meaning that rendering on the client will always use a property instead of an attribute for className. Without this, one of the automated tests was getting errors (I think during hydration?) due to this mismatch.
Alternatively we could make it so that when client rendering className is always set as an attribute instead of a property, but I think this makes more sense and it's what Preact is already doing.
There was a problem hiding this comment.
Yea this make sense because htmlFor doesn't have any semantics on custom element which are just Generic HTML elements.
There are a number of other ones that are case sensitive for similar reasons. contentEditable, tabIndex, etc. They probably work if used correctly, but it would be nice to add a warning if they use the wrong case.
There are some that just won't work like offsetHeight etc. Since those exist as properties but are read-only. Probably should ensure we have a nice warning about those and why they don't work. So that if they're rendered on the server, it doesn't look like they work.
Similarly there are some we have to be careful with like innerHTML and innerText. It shouldn't pass through as a property to custom elements - even in production so it can't be a DEV only error. Could be worth a test.
There was a problem hiding this comment.
I'm surprised innerText and textContent aren't reserved properties in React. Doesn't seem right.
There was a problem hiding this comment.
There are a number of other ones that are case sensitive for similar reasons. contentEditable, tabIndex, etc. They probably work if used correctly, but it would be nice to add a warning if they use the wrong case.
contentEditable is interesting, it looks like the attribute can be either contentEditable or contenteditable, they both work, and the reflected object property is contentEditable.
So I guess if someone writes <my-custom-element contentEditable={true} /> or <my-custom-element contenteditable={true} /> they should both work fine, right...?
There are some that just won't work like offsetHeight etc. Since those exist as properties but are read-only. Probably should ensure we have a nice warning about those and why they don't work. So that if they're rendered on the server, it doesn't look like they work.
I mean, you can assign to offsetHeight from script and the browser doesn't complain about it... but yeah I guess it would be weird to have a state where you can assign to an attribute called offsetHeight when rendering on the server, but you can't on the client because it will always trigger the 'offsetHeight' in node check...
Should I consider every global attribute and every built in property?
There was a problem hiding this comment.
For the read only properties like offsetHeight, I just pushed a commit which emits the warning: 1a093e5
However, the test fails because it throws an exception when assigning to the read only properties - but this exception doesn't occur in the browser or in JSDOM... what kind of browser/DOM setup is being used in these tests?
There was a problem hiding this comment.
“use strict” throws, right?
There was a problem hiding this comment.
Oh wow, today I learned...
What do you think we should do? Remove the test? Make it somehow get rid of the 'use strict'?
There was a problem hiding this comment.
I believe if you render it on the server and hydrate it wouldn’t error.
The reason for the warning would be so that if you render it on the server, during development you should know not to.
So you can test hydrating it.
There was a problem hiding this comment.
Ah, that makes sense!
I replaced the client rendering check with a hydration check and made a test for it.
| node: Element) { | ||
| if (name in (node: any)) { | ||
| const acceptsBooleans = (typeof (node: any)[name]) === 'boolean'; | ||
| return { |
There was a problem hiding this comment.
This seems a bit slow to do for every property. Likely will need some refactoring. However, is this check also equivalent on the built-ins?
There was a problem hiding this comment.
Are you referring to name in (node: any) or (typeof (node: any)[name]) === 'boolean'?
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
This is the last document I made: https://docs.google.com/document/d/1wwL_YZ5TnYorEEr2Lkeh9rgbnsynDXXI7tCejVcpE6I/edit# I added another test for the behavior I discussed in the doc about not removing attributes when assigning to properties.
I added some tests for this, thanks!
I skimmed the patch as well as the doc, and I believe everything is being tested now |
seems reasonable |
Summary: This sync includes the following changes: - **[f2a59df48](facebook/react@f2a59df48 )**: Remove unstableAvoidThisFallback from OSS ([#22884](facebook/react#22884)) //<salazarm>// - **[24dd07bd2](facebook/react@24dd07bd2 )**: Add custom element property support behind a flag ([#22184](facebook/react#22184)) //<Joey Arhar>// - **[72e48b8e1](facebook/react@72e48b8e1 )**: Fix: Don't skip writing updated package.json //<Andrew Clark>// - **[e39b2c899](facebook/react@e39b2c899 )**: Fix peer deps for use-sync-external-store //<Andrew Clark>// - **[ec78b135f](facebook/react@ec78b135f )**: Don't override use-sync-external-store peerDeps ([#22882](facebook/react#22882)) //<Andrew Clark>// - **[5041c37d2](facebook/react@5041c37d2 )**: Remove hydrate option from createRoot ([#22878](facebook/react#22878)) //<salazarm>// - **[3f9480f0f](facebook/react@3f9480f0f )**: enable continuous replay flag ([#22863](facebook/react#22863)) //<salazarm>// - **[4729ff6d1](facebook/react@4729ff6d1 )**: Implement identifierPrefix option for useId ([#22855](facebook/react#22855)) //<Andrew Clark>// - **[ed00d2c3d](facebook/react@ed00d2c3d )**: Remove unused flag ([#22854](facebook/react#22854)) //<Dan Abramov>// - **[0cc724c77](facebook/react@0cc724c77 )**: update ReactFlightWebpackPlugin to be compatiable with webpack v5 ([#22739](facebook/react#22739)) //<Michelle Chen>// - **[4e6eec69b](facebook/react@4e6eec69b )**: fix: document can be `null`, not just `undefined` ([#22695](facebook/react#22695)) //<Simen Bekkhus>// Changelog: [General][Changed] - React Native sync for revisions c1220eb...a049aa0 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D33062386 fbshipit-source-id: 37e497947efad5696c251096da8a92ccdc6dcea7
* custom element props * custom element events * use function type for on* * tests, htmlFor * className * fix ReactDOMComponent-test * started on adding feature flag * added feature flag to all feature flag files * everything passes * tried to fix getPropertyInfo * used @GATE and __experimental__ * remove flag gating for test which already passes * fix onClick test * add __EXPERIMENTAL__ to www flags, rename eventProxy * Add innerText and textContent to reservedProps * Emit warning when assigning to read only properties in client * Revert "Emit warning when assigning to read only properties in client" This reverts commit 1a093e5. * Emit warning when assigning to read only properties during hydration * yarn prettier-all * Gate hydration warning test on flag * Fix gating in hydration warning test * Fix assignment to boolean properties * Replace _listeners with random suffix matching * Improve gating for hydration warning test * Add outerText and outerHTML to server warning properties * remove nameLower logic * fix capture event listener test * Add coverage for changing custom event listeners * yarn prettier-all * yarn lint --fix * replace getCustomElementEventHandlersFromNode with getFiberCurrentPropsFromNode * Remove previous value when adding event listener * flow, lint, prettier * Add dispatchEvent to make sure nothing crashes * Add state change to reserved attribute tests * Add missing feature flag test gate * Reimplement SSR changes in ReactDOMServerFormatConfig * Test hydration for objects and functions * add missing test gate * remove extraneous comment * Add attribute->property test
|
This is not flexible enough for custom element users (#11347 (comment)). In today's modern Custom Elements world, users need choice:
This is why Lit's I've experienced all of these use cases in at most two apps. They are not rare. Here's a Lit example: return html`
<some-element
?some-boolean=${this.someBoolean}
another-booolean
some-attribute=${this.someString}
.some-property=${this.someNonString}
></some-element>
`Solid uses For the above reasons, this solution does not meet the requirements that custom element users have, and this solution is incomplete by today's standards. Custom element users need a complete solution to this. If not with syntax, then with something runtime like "directives". F.e. |
|
@jfbrennan since you were curious, see that ☝️. Custom element users must be responsible for knowing how custom elements are used, including whether to set an attribute or a property, and no system can accurately determine this. We're still gonna need to resort to I bet someone can solve this for 100% of the needed cases by making an |
|
That's too bad, but actually a good thing long term. Proprietary frameworks that consider themselves the center of the universe will die, those that have already caught up with the needs of the WC era will live on. |
|
Maybe I'm making a wrong assumption, but I installed |
|
Can you provide a minimal example? |
|
Hi @josepharhar , I'm reporting a potential bug I encountered while testing custom elements with the latest versions of react@experimental and react-dom@experimental. Observed behavior: I created a custom element with a callback function bound to a custom event. Example: When I attach the event listener directly without using react, the callback is getting triggered. |
|
@Amrender-Singh I can't get this to work with plain HTML either: https://stackblitz.com/edit/vitejs-vite-vuvkyc?file=index.html,customElement.js I believe the problem is that you're dispatching the custom event at |
|
@trusktr The cases you believe are currently not covered are best raised in a new issue for visibility. Though I believe they can be covered by how React implements custom elements in a way that seems to me more like idiomatic HTML:
Isn't the solution here either using Custom states and custom state pseudo-class CSS selectors or attribute reflection? Just like you must use
You'd have to use a ref here for built-in browser components as well, no? Seems like this is a feature request that's not specific to custom elements. |
|
@eps1lon It is working fine in HTML+JS, the main issue with your example was you were not listening to the event correctly, I think in order to listen for custom events using vanilla JS you need to register listener using addEventListener. https://open-wc.org/guides/knowledge/events/ Example: |
|
The event handler for custom elements is case sensitive. So if you dispatch I'll search for some more context in the spec and other frameworks. Though at first I would expect that You should also drop the |
|
Not sure if we should special case the first letter here and map The main confusion here was that React removes the |
|
How does this work with events that use dashes or colons?
|
events with dashes already have a test: https://github.com/facebook/react/pull/22184/files#diff-db98d094dc4f5b1a9c32b4da4df310370707408a929762868d44bf1acd515923R188 events with colon do not work as JSX attribute due to not being supported by React's JSX though we don't have a test if they're spread to props as an object e.g. |
That's one possible solution that a custom element author (not the user) has.
As a custom elements user (not an author) there is no guarantee that every author of all custom elements that ever exist will follow any rules specifically for React, so manual control is needed. Because of this, any templating system that exists (not only React JSX) and supports custom elements needs to give users the ability to choose whether they set HTML attributes vs JS properties. Lit's See here for a better description of the problem and how Lit and Solid support use cases: See here for the
Sometimes you just need to set a JS property and not an attribute, for whatever reason (you might not need a ref at all, you just want to set the JS property in the JSX, and something else like a jQuery plugin outside of the component could be reading the JS properties). "Use a ref" essentially means "use plain JS (instead of React JSX)". Plain JS has always worked, but that defeats the goal here, which is to not have to break out of React JSX into plain JS (because that's much more convenient for the developer experience). |
Summary
This implements the solution I've been proposing and discussing for #11347 which does an "in" check on the custom element to determine if we should be assigning to the element's property instead of its attribute, as well as adding support for custom events by making JSX attributes which start with "on" event listeners.
This doesn't implement the whenDefined -> reapply properties behavior we have been discussing, I'm planning on addressing that later.
Test Plan
I've been keeping my test site up to date and this patch includes several automated tests, but I still haven't gotten around to using the
fixtures/attribute-behaviortest to look for more behavior changes yet.