facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
226.65k stars 46.23k forks source link

RFC: Plan for custom element attributes/properties in React 19 #11347

Open robdodson opened 6 years ago

robdodson commented 6 years ago

This is meant to address #7249. The doc outlines the pros and cons of various approaches React could use to handle attributes and properties on custom elements.

TOC/Summary

Background

When React tries to pass data to a custom element it always does so using HTML attributes.

<x-foo bar={baz}> // same as setAttribute('bar', baz)

Because attributes must be serialized to strings, this approach creates problems when the data being passed is an object or array. In that scenario, we end up with something like:

<x-foo bar="[object Object]">

The workaround for this is to use a ref to manually set the property.

<x-foo ref={el => el.bar = baz}>

This workaround feels a bit unnecessary as the majority of custom elements being shipped today are written with libraries which automatically generate JavaScript properties that back all of their exposed attributes. And anyone hand-authoring a vanilla custom element is encouraged to follow this practice as well. We'd like to ideally see runtime communication with custom elements in React use JavaScript properties by default.

This doc outlines a few proposals for how React could be updated to make this happen.

Proposals

Option 1: Only set properties

Rather than try to decide if a property or attribute should be set, React could always set properties on custom elements. React would NOT check to see if the property exists on the element beforehand.

Example:

<x-foo bar={baz}>

The above code would result in React setting the .bar property of the x-foo element equal to the value of baz.

For camelCased property names, React could use the same style it uses today for properties like tabIndex.

<x-foo squidInk={pasta}> // sets .squidInk = pasta

Pros

Easy to understand/implement

This model is simple, explicit, and dovetails with React’s "JavaScript-centric API to the DOM".

Any element created with libraries like Polymer or Skate will automatically generate properties to back their exposed attributes. These elements should all "just work" with the above approach. Developers hand-authoring vanilla components are encouraged to back attributes with properties as that mirrors how modern (i.e. not oddballs like <input>) HTML5 elements (<video>, <audio>, etc.) have been implemented.

Avoids conflict with future global attributes

When React sets an attribute on a custom element there’s always the risk that a future version of HTML will ship a similarly named attribute and break things. This concern was discussed with spec authors but there is no clear solution to the problem. Avoiding attributes entirely (except when a developer explicitly sets one using ref) may sidestep this issue until the browsers come up with a better solution.

Takes advantage of custom element "upgrade"

Custom elements can be lazily upgraded on the page and some PRPL patterns rely on this technique. During the upgrade process, a custom element can access the properties passed to it by React—even if those properties were set before the definition loaded—and use them to render initial state.

Custom elements treated like any other React component

When React components pass data to one another they already use properties. This would just make custom elements behave the same way.

Cons

Possibly a breaking change

If a developer has been hand-authoring vanilla custom elements which only have an attributes API, then they will need to update their code or their app will break. The fix would be to use a ref to set the attribute (explained below).

Need ref to set attribute

By changing the behavior so properties are preferred, it means developers will need to use a ref in order to explicitly set an attribute on a custom element.

<custom-element ref={el => el.setAttribute('my-attr', val)} />

This is just a reversal of the current behavior where developers need a ref in order to set a property. Since developers should rarely need to set attributes on custom elements, this seems like a reasonable trade-off.

Not clear how server-side rendering would work

It's not clear how this model would map to server-side rendering custom elements. React could assume that the properties map to similarly named attributes and attempt to set those on the server, but this is far from bulletproof and would possibly require a heuristic for things like camelCased properties -> dash-cased attributes.

Option 2: Properties-if-available

At runtime React could attempt to detect if a property is present on a custom element. If the property is present React will use it, otherwise it will fallback to setting an attribute. This is the model Preact uses to deal with custom elements.

Pseudocode implementation:

if (propName in element) {
  element[propName] = value;
} else {
  element.setAttribute(propName.toLowerCase(), value);
}

Possible steps:

Pros

Non-breaking change

It is possible to create a custom element that only uses attributes as its interface. This authoring style is NOT encouraged, but it may happen regardless. If a custom element author is relying on this behavior then this change would be non-breaking for them.

Cons

Developers need to understand the heuristic

Developers might be confused when React sets an attribute instead of a property depending on how they’ve chosen to load their element.

Falling back to attributes may conflict with future globals

Sebastian raised a concern that using in to check for the existence of a property on a custom element might accidentally detect a property on the superclass (HTMLElement).

There are also other potential conflicts with global attributes discussed previously in this doc.

Option 3: Differentiate properties with a sigil

React could continue setting attributes on custom elements, but provide a sigil that developers could use to explicitly set properties instead. This is similar to the approach used by Glimmer.js.

Glimmer example:

<custom-img @src="corgi.jpg" @hiResSrc="corgi@2x.jpg" width="100%">

In the above example, the @ sigil indicates that src and hiResSrc should pass data to the custom element using properties, and width should be serialized to an attribute string.

Because React components already pass data to one another using properties, there would be no need for them to use the sigil (although it would work if they did, it would just be redundant). Instead, it would primarily be used as an explicit instruction to pass data to a custom element using JavaScript properties.

h/t to @developit of Preact for suggesting this approach :)

Pros

Non-breaking change that developers can opt-in to

All pre-existing React + custom element apps would continue to work exactly as they have. Developers could choose if they wanted to update their code to use the new sigil style.

Similar to how other libraries handle attributes/properties

Similar to Glimmer, both Angular and Vue use modifiers to differentiate between attributes and properties.

Vue example:

<!-- Vue will serialize `foo` to an attribute string, and set `squid` using a JavaScript property -->
<custom-element :foo="bar” :squid.prop=”ink”>

Angular example:

<!-- Angular will serialize `foo` to an attribute string, and set `squid` using a JavaScript property -->
<custom-element [attr.foo]="bar” [squid]=”ink”>

The system is explicit

Developers can tell React exactly what they want instead of relying on a heuristic like the properties-if-available approach.

Cons

It’s new syntax

Developers need to be taught how to use it and it needs to be thoroughly tested to make sure it is backwards compatible.

Not clear how server-side rendering would work

Should the sigil switch to using a similarly named attribute?

Option 4: Add an attributes object

React could add additional syntax which lets authors explicitly pass data as attributes. If developers do not use this attributes object, then their data will be passed using JavaScript properties.

Example:

const bar = 'baz';
const hello = 'World';
const width = '100%';
const ReactElement = <Test
  foo={bar} // uses JavaScript property
  attrs={{ hello, width }} // serialized to attributes
/>;

This idea was originally proposed by @treshugart, author of Skate.js, and is implemented in the val library.

Pros

The system is explicit

Developers can tell React exactly what they want instead of relying on a heuristic like the properties-if-available approach.

Extending syntax may also solve issues with event handling

Note: This is outside the scope of this document but maybe worth mentioning :)

Issue #7901 requests that React bypass its synthetic event system when declarative event handlers are added to custom elements. Because custom element event names are arbitrary strings, it means they can be capitalized in any fashion. To bypass the synthetic event system today will also mean needing to come up with a heuristic for mapping event names from JSX to addEventListener.

// should this listen for: 'foobar', 'FooBar', or 'fooBar'?
onFooBar={handleFooBar}

However, if the syntax is extended to allow attributes it could also be extended to allow events as well:

const bar = 'baz';
const hello = 'World';
const SquidChanged = e => console.log('yo');
const ReactElement = <Test
  foo={bar}
  attrs={{ hello }}
  events={{ SquidChanged}} // addEventListener('SquidChanged', …)
/>;

In this model the variable name is used as the event name. No heuristic is needed.

Cons

It’s new syntax

Developers need to be taught how to use it and it needs to be thoroughly tested to make sure it is backwards compatible.

It may be a breaking change

If any components already rely on properties named attrs or events, it could break them.

It may be a larger change than any of the previous proposals

For React 17 it may be easier to make an incremental change (like one of the previous proposals) and position this proposal as something to take under consideration for a later, bigger refactor.

Option 5: An API for consuming custom elements

This proposal was offered by @sophiebits and @gaearon from the React team

React could create a new API for consuming custom elements that maps the element’s behavior with a configuration object.

Pseudocode example:

const XFoo = ReactDOM.createCustomElementType({
  element: ‘x-foo’,
  ‘my-attr’: // something that tells React what to do with it
  someRichDataProp: // something that tells React what to do with it
});

The above code returns a proxy component, XFoo that knows how to pass data to a custom element depending on the configuration you provide. You would use this proxy component in your app instead of using the custom element directly.

Example usage:

<XFoo someRichDataProp={...} />

Pros

The system is explicit

Developers can tell React the exact behavior they want.

Non-breaking change

Developers can opt-in to using the object or continue using the current system.

Idiomatic to React

This change doesn’t require new JSX syntax, and feels more like other APIs in React. For example, PropTypes (even though it’s being moved into its own package) has a somewhat similar approach.

Cons

Could be a lot of work for a complex component

Polymer’s paper-input element has 37 properties, so it would produce a very large config. If developers are using a lot of custom elements in their app, that may equal a lot of configs they need to write.

May bloat bundle size

Related to the above point, each custom element class now incurs the cost of its definition + its config object size.

Note: I'm not 100% sure if this is true. Someone more familiar with the React build process could verify.

Config needs to keep pace with the component

Every time the component does a minor version revision that adds a new property, the config will need to be updated as well. That’s not difficult, but it does add maintenance. Maybe if configs are generated from source this is less of a burden, but that may mean needing to create a new tool to generate configs for each web component library.

cc @sebmarkbage @gaearon @developit @treshugart @justinfagnani

yananym commented 3 years ago

@gaearon yep that's what i would expect to happen in this situation.

justinfagnani commented 3 years ago

On the other hand, it's been three years, and I think you're saying this consensus has not formed yet

I'm not saying consensus hasn't been met. I'm saying you don't need to worry about deep SSR right now, and shouldn't let pretty vague concerns about it block the most basic client-side interoperability that can be accomplished and be extremely useful right now.

gaearon commented 3 years ago

I haven't seen this distinction before (deep vs shallow) so let me try to restate it to check if I understood you.

By "deep" SSR, I think you mean SSR similar to how it works in React components. That is, rendering a CE would produce an output that can be displayed before that CE's logic has loaded. This is something you're saying we should not worry about supporting now. That makes sense to me.

By "not deep" SSR, I think you're saying that we just put the CE tag itself into HTML. Without worrying what it resolves to. That makes sense to me too. My question was about this flow though — not about the "deep" SSR.

In particular, https://github.com/facebook/react/issues/11347#issuecomment-713230572 describes a situation where even when we already have the CE logic downloaded, we still can't show anything to the user until hydration. Becomes its properties are unknown until the whole application's JS client code loads and hydration happens. I'm asking whether this is indeed the desired behavior.

I don't think it's a vague concern from my perspective. I don't want to misunderstand the feature request. So getting the exact semantics specified would help me evaluate this proposal. E.g. one practical concern is that React doesn't actually diff attributes/properties in production during hydration today because none of the built-in components need that. But it's something we could add for this particular case if it's really needed.

alangdm commented 3 years ago

@gaearon

I think https://github.com/facebook/react/issues/11347#issuecomment-713230572 might not be the best example case in which this feature is needed.

At least in my experience, setting properties rather than attributes is not all that necessary for simpler types like strings, numbers, or booleans.

The case you describe could easily be achieved just using iconname as an attribute directly and it would still be largely the same final rendered result without waiting for hydration.

The custom element implementation loads and it gets registered. If I understand correctly, this is the process called "upgrading". However, even though its JS is ready, it still can't show anything because we have not included the iconname in the HTML. So we don't have the information for which icon to render. This is because we said earlier that "non-builtin properties can be dropped from the HTML". So the user still sees a "hole".

As for what you mention here, depending on how the component is implemented you could avoid this problem of seeing a "hole".

That could be achieved either by just plain setting defaults in this case or by accepting part or even most of the content through slots so that content is displayed even before the component is upgraded.


I think this feature would be mostly useful for using more complex components that have properties which are Objects, Arrays, Functions, etc.

Take for an example, the lit-virtualizer virtual scroller component.

For this to work properly it requires an items array and a renderItem function and you can even optionally set an scrollTarget Element, all of which can only be set in React using refs right now.

For a case like this, you would probably be loading the content with some sort of pagination or lazy loading anyway so not having any content until the hydration step on an SSR case might not be that problematic.

TimvdLippe commented 3 years ago

@gaearon Please note that custom elements are a DOM standard and therefore there isn't per se a "WC community" in the sense that everyone who uses custom elements uses them in the same way. Custom elements are integrated in their own way for each developer, as they are inherently a low-level primitive that people build upon.

That said, over time numerous patterns have emerged and all popular frameworks (except React) implement some solution to provide compatibility for this DOM standard. As you can see on https://custom-elements-everywhere.com/, all frameworks/libraries (except React) have chosen different options. In this issue, the chosen options are listed as options 1, 2 and 3. There is no framework/library that currently implements option 4 or 5 and I don't think it is desirable to implement these.

Therefore, I propose that React follows suit with the other frameworks/libraries and chooses an option that has proven working, e.g. out of options 1-3. I don't think React needs to reinvent the wheel here by choosing option 4 or 5, which we have no data for is a longterm maintainable solution for either React or those that build upon DOM standards.

karlhorky commented 3 years ago

So, by the process of elimination (with the comments linked below), since option 3 is not going to be done to JSX and options 4 and 5 have been marked undesirable by WC people in this thread, we're stuck with 1 or 2.

And since 1 seems like it has untenable drawbacks, it looks like Option 2 is the one? Just go with how Preact does it?


There is no framework/library that currently implements option 4 or 5 and I don't think it is desirable to implement these.

(@TimvdLippe in https://github.com/facebook/react/issues/11347#issuecomment-713474037)

I think it's pretty unlikely that we would introduce new JSX syntax for the sake of this particular feature alone.

(@gaearon in https://github.com/facebook/react/issues/11347#issuecomment-713210204)

TimvdLippe commented 3 years ago

That would make sense to me yes. Thanks for the summary! 😄

gaearon commented 3 years ago

At least in my experience, setting properties rather than attributes is not all that necessary for simpler types like strings, numbers, or booleans. The case you describe could easily be achieved just using iconname as an attribute directly and it would still be largely the same final rendered result without waiting for hydration.

I'm not quite following. My hypothetical scenario was a response to https://github.com/facebook/react/issues/11347#issuecomment-713223628 which sounded like a suggestion that we shouldn't emit attributes in the server-generated HTML at all, except for id and class. I don't know what "just using iconname as an attribute" means — are you saying you'd like to see the server-generated HTML include other attributes? This seems to contradict what was just said earlier. I think it would really help if each response presented a complete picture because otherwise we're going to keep going in circles. My question to you is:

  1. What exactly gets serialized in the server-generated HTML in the scenario I described a. In case <github-icon iconname="smiley" /> b. In the case you brought up, e.g. <github-icon icon={{ name: "smiley" }} />
  2. What DOM API is called on the client during hydration in either of those cases

Thanks!

just-boris commented 3 years ago

Was it considered an option to create a shim, replacing React.createElement() with a custom implementation, which does properties mapping inside?

Usage example:

/** @jsx h */
import { h } from 'react-wc-jsx-shim';

function Demo({ items }) {
   return <my-custom-list items={items} />
}

Draft implementation:

export function h(element, props, children) {
   if(typeof element === 'string' && element.includes('-')) {
      return React.createElement(Wrapper, { props, customElementName: element }, children)
   }
   return React.createElement(element, props, children);

}

function Wrapper({customElementName, props}) {
   const ref = React.useRef();
   React.useEffect(() => {
      for(const prop in props) {
         ref.current[prop] = props[prop];
      }
   });
   return React.createElement(customElementName, { ref, ...props });
}

I know that this is not a built-in feature, but should unblock users with very low overhead, I think.

justinfagnani commented 3 years ago

@just-boris that is basically what apps or libraries do now, with either a createElement patch or a wrapper. You also need to exclude the special-cased property names in React. I don't think there's an exported list, so they just hard code a denylist like ['children', 'localName', 'ref', 'elementRef', 'style', 'className'].

Because it's ref-based, the properties are not set on the server. This is why I call the concern vague. There's no way to set properties at all in React, so it's all broken right now. The workaround available already only work on the client. If there becomes built-in a way to set properties on the client and not on the server, it doesn't suddenly fix or break SSR.

I would actually be more concerned with events than SSR. React doesn't have a way to add event handlers, and that's a huge hole in interoperability with basic DOM APIs.

treshugart commented 3 years ago

Was it considered an option to create a shim, replacing React.createElement() with a custom implementation, which does properties mapping inside?

As mentioned by Rob in the original issue, I've experimented with this in the past via https://github.com/skatejs/val, so having a custom JSX pragma that wraps React.createElement is a viable - possibly short-term - solution.

I also have WIP branch of Skate implementing both of what has been referred to as deep and shallow SSR, specifically for React. The learnings from this work thus far, are that you can't have a single JSX wrapper for all libraries if you want to do deep SSR because each library is dependent on their own APIs and algorithms for doing so. (For context, Skate has a core custom element part of its library and small layers built to integrate each popular library on the market to make consumption and usage consistent across the board.)


I don't operate much in open source anymore, and don't do Twitter anymore either, so feel free to take my opinion with a grain of salt.

Option 1 feels like it exists as the devil's advocate for Option 2.

Option 2 feels like the way to go. It's close to what Preact has done, so there's precedent and a possibility of a community standard (which I think would be fantastic to do anyways; JSX has shown this can work), and it's also a pragmatic move for React in being easy to upgrade (as opposed to Option 1).

Option 3 looks good on paper, but I think logistically it would be a lot more difficult than Option 2 due to cognitive overhead, documenting around said overhead, and unknowns with SSR.

As mentioned, I had originally presented option 4 but after reflection, I'm not sure I like it anymore. You could change it to be more opt-in, where you specify explicit props and events (instead of attrs), but even then Option 2 requires less knowledge of implementation details and there's nothing for anyone to have to change to adopt it.

Option 5 feels like we'd be ignoring the core issue entirely.


As an aside, I'm super happy to see traction on this and hope y'all have been doing well!

just-boris commented 3 years ago

If there is a possible solution in the userland, can it be consolidated and recommended by the community for the time being?

It would be much easier to convince the React core team to add a feature if it already exists as a popular and viable 3rd-party module.

claviska commented 3 years ago

If there is a possible solution in the userland, can it be consolidated and recommended by the community for the time being?

I'm don't believe this is something that can be [neatly] patched from the outside. Most userland solutions are wrappers such as this and this. Without modifying React, I can't think of a reasonable alternative to this approach, and wrapping is definitely not an ideal solution. 😕

graynorton commented 3 years ago

Note that in vuejs/vue#7582, Vue chose to use a sigil, and they chose "." as a prefix (not Glimmer's "@").

I don't have a link to an authoritative statement on this (and maybe someone closer to Vue can explicitly confirm), but it appears that Vue has moved to Option 2 as of Vue 3.0, meaning that it behaves similarly to Preact. (See this issue for context.)

gaearon commented 3 years ago

Because it's ref-based, the properties are not set on the server. This is why I call the concern vague. There's no way to set properties at all in React, so it's all broken right now. The workaround available already only work on the client. If there becomes built-in a way to set properties on the client and not on the server, it doesn't suddenly fix or break SSR.

Can you please respond to the specific questions in https://github.com/facebook/react/issues/11347#issuecomment-713514984? I feel like we're talking past each other. I just want to understand the whole intended behavior — in a specced way. What gets set when, and what gets serialized in which case.

brainlessbadger commented 3 years ago

As a random Preact+WC dev who stumbled here, I wanted to point out that "Option 2" is not only a breaking change, but also an incomplete solution (Preact can still only interact declaratively with a subset of valid Web Components).

Even as a current user of the heuristic, that sounds hardly desirable in the long run.


Preact's heuristic ("option 2") is unable to set properties that have special meaning in React (key, children, etc.) or Preact (anything starting with on).

I.e. after rendering below JSX:

<web-component key={'key'} ongoing={true} children={[1, 2]} />

properties key, ongoing and children will all be undefined (or whatever the default value is) on the created instance of web-component.

Also, contrary to the OP, this option is also a breaking change. Most web components (e.g. made with lit-element) offer ability to pass serialized rich data via attributes, for purpose of being used with pure HTML. Such components can be rendered from React like so

<web-component richdata={JSON.stringify(something)} />

which will break if "option 2" is chosen. Not sure how common such use of web components is in React, but there are definitely some code bases that do so, it's less efficient then refs but also terser.

Finally, Web Component's author is free to attach subtly different semantics to attribute and property of the same name. An example would be a Web Component mimicking native input element's behavior - treating attribute value as initial value, and only observing changes on the property value. Such components can't be fully interacted with via jsx with the heuristic approach, and also could experience a subtle breakage if it was introduced.

effulgentsia commented 3 years ago

I think it's pretty unlikely that we would introduce new JSX syntax for the sake of this particular feature alone.

What about using namespaces? https://github.com/facebook/jsx/issues/66 proposes to add a react namespace for key and ref. Along the same lines, would something like react-dom be an ok namespace to mint for identifying DOM properties? Using the OP's option 3 example, that would be:

<custom-img react-dom:src="corgi.jpg" react-dom:hiResSrc="corgi@2x.jpg" width="100%">

That's not the most ergonomic. Just dom would be better, but not sure if React wants to mint namespaces that don't start with react. Also, poor ergonomics on this isn't the end of the world, if the idea is that setting as properties should be the less common case. Using attributes is better, since that provides parity with SSR. Setting as properties is for when attributes are problematic (such as to pass an object or for properties not backed by attributes), and it is precisely for those problematic properties that we can't SSR them to attributes anyway, and would need to hydrate them via JS.

karlhorky commented 3 years ago

an incomplete solution

(Preact can still only interact declaratively with a subset of valid Web Components)

@brainlessbadger can you also edit your comment above to include what this subset actually is? Eg. which Web Components / Custom Elements are affected (with examples)? And how exactly they are affected?

brainlessbadger commented 3 years ago

@karlhorky I added an explanation. Sorry for being needlessly vague.

sebmarkbage commented 3 years ago

Regarding Web Components events. Is there any consensus yet around how to avoid future blocking by colliding names of events?

This is a problem for attributes and properties too since new attributes can be added to HTMLElement like all of these: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes Looks like there are a couple of newly added ones too so there are still new ones being added.

I think strictly speaking you should have to use a - in the name of any custom attribute or custom event.

Anything else is a compromise weighing the risks of that component blocking a future spec from using a nice name.

Events makes me extra nervous though because it's not just a risk of that component having incompatible behavior. If the new event bubbles, it'll trigger all existing Web Component in that tree path.

If custom attributes and events were required to use a - in their name that could be used as a heuristic to determine what to use. Then we'd special case the built-in ones. That's already how we know if it's a custom element in the first place.

Properties could then be used as conveniences to provide a nicer short hand. At least those can shadow the built-in properties and not collide with future properties.

claviska commented 3 years ago

I think strictly speaking you should have to use a - in the name of any custom attribute or custom event.

The custom element spec doesn't include such restrictions, so enforcing them in React will significantly affect interoperability.

Custom element authors shouldn't be expected to subscribe to a framework's arbitrary requirements. Imagine every framework doing this with varying opinions and conventions. This defeats the purpose of using web components. 😕

Ideally, the framework should adapt to accommodate the standard.

justinfagnani commented 3 years ago

Indeed. setAttribute() and dispatchEvent() are just non-web-component specific APIs that have allowed arbitrary names since their inception. Any element can have any attribute and fire any event - it's just a ground truth of the DOM.

sebmarkbage commented 3 years ago

The data- namespace was added for a reason though. There’s a difference between what you technically could do and what’s best practice. You can technically patch prototypes too but then you get contains/includes situations.

I wouldn’t suggest that just React adds this but that the community shifts to this convention to help protect the future namespace. I just think it’s unfortunate that this issue isn’t taken seriously. But perhaps the web is doomed to only add awkward names to new APIs.

sebmarkbage commented 3 years ago

I like the idea of just excluding non-builtins from SSR and prefer properties during hydration because with declarative shadow DOM the real content could be expanded inside.

Concretely though I believe that will cause problem for the current approaches people use with SSR + AMP since that uses attributes and you can assume that the AMP runtime has already loaded.

bahrus commented 3 years ago

Assuming the door isn't closed on option 3, it has a significant advantage over option 2, not listed in the cons above -- The problem I've found with option 2 is that if web component libraries are loaded asynchronously, preact might not know, for an unknown element, that a "property" will be a property. Since it doesn't find the property existing in the unknown element, it defaults to an attribute. A work around is to not render that component, until the dependent web component libraries are loaded, which isn't very elegant (or ideal performance-wise). Since my group uses asp.net, not node.js, I've never explored SSR really, so the observations below are speculative.

It's a nice gesture, I think, on the React team's part, to suggest going above and beyond what other frameworks support (to my knowledge), as far as SSR, allowing object properties to be passed down during initial rendering, which could serve the user better.

I don't know if I'm stating the obvious or not, but I think there is a bit of a consensus with web components that for some properties, it is convenient to sometimes set the initial value of the attribute via a JSON string, wrapped in single quotes.

AMP uses another convention, though -- it uses script type=application.json to do this, which is a reasonable (but verbose) alternative.

But sticking with the attribute approach:

The web component library can then parse the attribute, or be passed in the value via a property.

So to avoid hydration delays it would be convenient to set:

<github-icon icon='{"name":"smiley"}'></github-icon>

during SSR. Then github-icon could immediately do its thing, internally do a JSON.parse of the attribute, and not have to wait for React to pass in the (more complete?) object value.

So now we have a tricky situation -- if rendering on the server, we want "icon" to be treated as an attribute, but be able to pass in the value as an object, which ideally the SSR mechanism would stringify into the attribute. On the client we want to be able to pass in the object directly, and not go through the overhead of stringifying and parsing.

Of course, some properties are objects that aren't amenable to stringifying/parsing, so this doesn't apply, and those properties would need to wait for hydration to complete.

If all properties and attributes followed the React team's preferred naming convention (perhaps) -- all attributes had dashes, and all properties were compound names using camelCase, perhaps the existence of a dash could help distinguish between attributes and properties:

<github-icon my-icon={myStringifiedProperty} myIcon={myObjectProperty}></github-icon>

The problem is nothing in the syntax above indicates what to do on the server versus the client, and ideally we would be able to use one binding for both, and React would be smart enough to figure out which one to use.

React could just assume that if an attribute/property key is camel case, to always pass it as an object on the client side, and a JSON-stringified serialization of the object if it's set during SSR. Likewise, dashes in the key could (safely, I think) assume it is an attribute, and just pass in the .toString() of the value. But I think this assumes too much. And not supporting single word attributes and properties, which is considered valid HTML if the attributes are applied to a web component extending HTMLElement, would be too constricted. I'm in favor of W3C issuing a list of "reserved" attribute names it might use in the future, similar to reserved key words for JS, and frameworks finding ways of warning developers if they are improperly using a reserved attribute name.

But I think option 3 is the best approach. However, if it can be enhanced, as suggested by @gaearon, for an even better user experience, that would be great.

My suggestion would be:

<github-icon icon={myDynamicIcon}/>

means attribute (toString()).

<github-icon icon:={myDynamicIcon}/>

would mean -- during SSR, ignore, but bind on the client to the object property (after hydrating).

Now what about the scenario (some of the?) React team is interested in solving? My first thought was just some other sigil, such as two colons:

<github-icon icon::={myDynamicIcon}/> //Not my final suggestion!

would mean, during SSR, JSON.stringify the property, set as an attribute in the rendered HTML, and pass as an object on the client when the property it is binding to changes after hydration.

This leaves the tricky situation of what to do with compound names. I.e. if we set:

<github-icon iconProps::={myDynamicIconProp}/>  //Not my final suggestion!

it wasn't controversial in the past that the corresponding attribute for property name iconProps should be icon-props.

Perhaps this has become more controversial, because of the specter that some of these web components could, with no changes, become built into the platform, where attributes can't have dashes (but properties can be camelCase). To my knowledge, there is not yet a native element that allows passing in complex objects via JSON deserializing, but I wouldn't be surprised if the need arises in the future. So how would React know when to insert dashes or not?

The only (ugly?) suggestion I can come up with is:

<github-icon icon-props:iconProps={myDynamicIconProp}/>

which means, on the server, use attribute icon-props after serializing, and on the client use property iconProps, passing in objects directly.

Another (long shot?) potential benefit of the more verbose notation, allowing for a hybrid attribute / property pair, is this: It might be a little faster to repeatedly set the properties of a native element, rather than the corresponding attribute, especially for properties that aren't strings. If so, does React currently use attributes on the server, and properties on the client? If not, is it because of the same issue of naming translation difficulties, which this notation would solve?

eavichay commented 3 years ago

@bahrus I think it can be simplified

<my-element attr='using-quotes' property={thisIsAnObject} />
bahrus commented 3 years ago

I think it might be best to illustrate the issue with a concrete example.

The HTML Form element has a number of attributes. The ones with "compound names" don't all seem to follow a consistent naming pattern -- generally it sticks with all lower case, no separators, but sometimes there's a dash separator -- "accept-charset" for example, sometimes not -- "novalidate".

The corresponding JS property name doesn't fit into a nice universal pattern either -- acceptCharset, noValidate. The noValidate property / novalidate attribute is a boolean, so on the client, it would be wasteful (I imagine) to do myForm.setAttribute('novalidate', '') as opposed to myForm.noValidate = true.

However, on the server, it doesn't make sense to set myForm.noValidate = true, as we want to send a string representation of the DOM, so we need to use the attribute:

<form novalidate={shouldNotValidate}>

Actually, it isn't clear to me that React/JSX has a universal way of setting a boolean attribute conditionally, relying, perhaps, on a fixed, static lookup table, which seems(?) overly rigid. If I may be so bold, this seems like another area React/JSX could improve, to be fully compatible with the (messy) way the DOM works, as part of this RFC?

How can we represent all these scenarios, so that the developer has full control on the server (via attributes) and client (via properties), without sacrificing performance? In this case of a boolean like novalidate, I would propose:

<form novalidate:noValidate?={shouldNotValidate}/>

If React fully supported the DOM, messy as it is, in as performant a way as possible, I think that would go a long way to support custom elements, which also are probably not very consistent in naming patterns.

Adding support for optionally serializing object properties to attributes via JSON.stringify on the server, would be an additional huge win-win for React and web components, it seems to me.

Or am I missing something? ( @eavichay , how would you suggest these scenarios best be represented, not following from your example).

eavichay commented 3 years ago

Or am I missing something? ( @eavichay , how would you suggest these scenarios best be represented, not following from your example).

I have implemented a "reactify" library for web components. It is done in an non "friendly" markup, but yet effective.

const properties = {
  disabled: true,
  items: [1,2,3,4],
};
<my-element some-attribute="whatever" Properties={properties} />

regarding boolean attributes, I have added the Attributes object bag which converts true to empty attribute, null or false to no-attributes, and others to strings, and dash-cases camelCased names

<my-element Attributes={{disabled: true, someId: 8}} />

Though JSX is a synthetic feature, at it's current point I can even compromise to the angular's style:

<my-element "enabled?"={true} "[someProp]"={someValue} />

This or lit-html's style using .prop. Regarding events, this is where the "react fun" starts. Let's start with the basics first.

trusktr commented 3 years ago

The data- namespace was added for a reason though. There’s a difference between what you technically could do and what’s best practice. You can technically patch prototypes too but then you get contains/includes situations.

Naming variables in JavaScript is not the same as patching prototypes. There's no strong convention (let alone enforcement) in JavaScript that prevents your function from having a variable with any name (even if it shadows outer scope variables), or your class from having any property names.

On that similar vein, there's really no need to have a convention to prevent an element from having its own properties (or attributes), even if those shadow builtins (or apply supplemental behavior to builtins).

I would suggest that the only thing that needs to be enforced is to ensure that Custom Element authors can easily override types of intrinsic attributes (props) for their custom intrinsic elements, which is not currently the case (TypeScript has an error if the type of a new IntrinsicElement's prop doesn't match one already defined in DetailedHTMLProps).


On another note, why is this taking so long? Why has the React team not prioritized web interop?

TimvdLippe commented 3 years ago

Per https://reactjs.org/blog/2021/06/08/the-plan-for-react-18.html I think we are going to have to update the issue title here to "React 19"

TimvdLippe commented 3 years ago

Today I saw that articles such as https://dev.to/bennypowers/what-s-not-new-in-react-18-45c7 are making the rounds. I understand that this issue has a long history of back and forth discussions, including a lot of assumptions from various people in the web community on multiple sides of this issue. To avoid further proliferation on assumptions on both the priorities and approach of the React core team, I think it would beneficial to clarify whether a commitment for React 19 can be in place or not.

If this issue is not on the near term roadmap of React (which as I read the published roadmaps I am currently assuming it is not), I think it would be best to clarify it in this issue as well. The issue was punted from React 17 and onwards, which is creating the expectation that it is solved sooner rather than later. However, if that is not the case, clarifying that would help everyone understand what the default position is. Even if this issue will not be tackled for a while, the web community can create unique wrappers for React to remedy this problem in the near term, while the React core team has the freedom to explore longterm solutions (if any).

Maybe a different way of saying: I don't think continuing a discussion in this GitHub issue specifically is helpful for anybody and can have material effect on the mental health of anybody involved. Especially for those that have to make the difficult decision on what to do (in this case the React core team), this GitHub issue has become a complicated space for constructive discussions and can more easily lead to increasingly negative responses.

Given the current state of affairs in the world, I don't think continuing on the path we have been up to this point is a healthy way forward. Clarifying the current position on this issue (even if the statement is "we will not be looking at this anytime soon") will probably be best for everyone involved. Thanks in advance!

just-boris commented 3 years ago

I think, this comment https://github.com/facebook/react/issues/11347#issuecomment-715382207 sums up the current point of the React team and why this topic has low priority in their plans.

P.S. I think it would be great to have some sort of pinned comment with a brief description of the current state to help folks who jump into the discussion without reading already existing 100+ comments

gaearon commented 3 years ago

Per https://reactjs.org/blog/2021/06/08/the-plan-for-react-18.html I think we are going to have to update the issue title here to "React 19"

React 18 is still at least several months away from general availability. We’re wrapping up several years of work in it so finishing that has been our primary focus. But we have a list of things to revisit when we get closer to a Beta, and revising this issue is on that list together with revisiting other breaking changes that we need to make a decision on.

There’s been some work on this issue around the end of 2020. We’ve been discussing various tradeoffs with Chrome engineers, and @josepharhar did a sketch of a possible implementation and also explored differences in the output in some edge cases. I’m not sure what the concrete next steps are (and whether this work is blocked on any input from our side) but I’ll raise this at our next meeting!

josepharhar commented 3 years ago

Hey everyone! I'm an engineer of the DOM team in Chrome.

As Dan mentioned above, I've been working on getting "option 2" implemented in React. One of the outstanding issues we still need to figure out is what to do when the custom element hasn't been upgraded yet, and what to do in response to the custom element becoming upgraded - the heuristic of looking at the custom element's object properties would cause us to assign JSX attributes to the element's HTML attributes instead if the custom element hasn't been upgraded yet. This issue also comes up in server-side rendering and hydration because we don't have custom element definitions/instances when doing server-side rendering, and the custom element may or may not be defined during hydration either...

Preact currently applies the "option 2" heuristic every time a JSX attribute being applied to a custom element changes (via setState for example), which means that if the custom element isn't upgraded before the first render(), the JSX attributes which should actually be set as properties will only be set as HTML attributes until the JSX attribute changes, at which time the new value for the JSX attribute will be assigned to the property (assuming the custom element is upgraded at this point), and the previous value will be left in the HTML attribute, which probably isn't a great state to leave the custom element in...

I wrote a doc exploring some of the possible ways to handle this, and I'd appreciate ideas anyone from this thread has, as well as pointing about anything I may have overlooked. https://docs.google.com/document/d/1PWm94eCKZ9OBe91X-3ZHIBxn_w0E92VrzSImTEmXPZc/edit#

bahrus commented 3 years ago

Although I am still rooting for option 3, on the grounds that, as a developer, I like to allow the developer to be in the driver's seat as much as possible, I would certainly welcome option 2 as a step forward.

It may be that today, "no one" [with the exception of me, under my breath :-)] is complaining about the asynchronous issue with preact, because most everyone uses out-of-the-box bundling, and follows official documentation examples, of which very few probably demonstrate use of dynamic imports, without any async/await/promise based callback logic), outside routing (just speculating without scouring the documentation). A search does turn up a flurry of unofficial blogs in the 2018 time period of use of dynamic imports, probably when dynamic imports was a shiny new toy. I would like to think that if preact/react had a good solution to this issue, they would document if officially, giving developers more confidence to give it a try, whatever "it" is.

My guess is the issue is only likely to arise for developers who "stick their finger in the fan" and try experimenting, against the grain, with this approach to code splitting. Low as the current volume of this kind of experimentation is today, I wouldn't be surprised if it increases with time (especially if React providing better support increases usage of React developers interfacing directly with web component libraries).

The options presented under option 2 seem well thought-out, and I'm not sure which is best. I guess option 1 seems to leave the developer most in the driver's seat (maybe?) so it sounds best to me.

I wonder if the right stopgap, within the confines of option 2, would be if somehow React could detect that a developer is using dynamic import, and nevertheless trying to use standard JSX prop binding -- maybe it could just throw an error, or at the very least, during development, issue a stern console warning, with suggested syntax along the lines of what @robdodson suggested:

<x-foo ref={el => {
   await customElements.whenDefined('x-foo');
   el.bar = baz
}>

(or whatever code is needed to wait for the custom element to be upgraded)?

Correction, actually the await wouldn't be necessary, if the custom element is able to upgrade properly (another big unknown in my mind -- what percentage can actually?).

I'm afraid I don't know how well this solution would work with SSR. So whatever the right solution is, maybe give a warning if the developers are clearly not using the right solution, with a link to the right solution, even if means the developer has to put in a little extra code/tender-loving-care?

gaearon commented 3 years ago

Since it was posted just before the weekend, I want to call some extra attention to the questions about hydration that @josepharhar has raised in https://github.com/facebook/react/issues/11347#issuecomment-877327383. Since lots of people appear to be in favor of Preact behavior (custom-elements-everywhere gives it a solid 100% score), is there any feedback on the inconsistencies in its hydration approach? Is it just that nobody currently seriously uses custom elements with SSR? Or how come this is not a bigger issue in practice?

morewry commented 3 years ago

I'm (production lol) dabbling in it with Next.js. Using its client-side-only dynamic imports for importing the custom element modules.

I've managed SSG & SSR (and general React compatibility) by implementing my custom elements so that all custom element inputs can be serialized as plain HTML. Inputs are HTML attribute strings/booleans and nested tags only. No inputs that need DOM properties: no objects, no arrays, no functions.

Because I already knew I couldn't use more complex input types without running into issues when integrating. Avoiding the question entirely. Yeah, it severely restricts my custom elements. And I would definitely like to be able to do more with them.

That's pretty much all I can say about it at the moment, not having had time to dig into SSR technical details. Maybe it doesn't come up as much as you'd expect because lots of people only use CSR and it is possible to work around it to some extent. People working on products don't usually have time to litigate library and platform behavior on this level. Just find a workaround and get it out the door. When I was in customer service, I assumed that for every single issue I heard about, I had 10 other customers silently bearing with the problem and 5 other issues I never heard about at all. Aside from that, I suppose everyone is here lurking and waiting on someone else to have a breakthrough. 😉

matthewp commented 3 years ago

@josepharhar @gaearon I don't think this is a problem that React is responsible for solving. There's a related known issue with custom elements where if their properties are set before the element is upgraded, any setters you have on the prototype will not run.

Here's a codepen demonstrating that issue: https://codepen.io/matthewp/pen/rNmjoLW?editors=1010. You can fix it by uncommenting the defineProperty in the constructor.

That is to say, upgrades are weird, and it's up to the custom element author to handle upgrade timing (if it wants to).

This comes down to the contract between the custom element and the page using the element. There are two solutions to this problem:

1) The custom element can handle properties possibly being set as either attributes or properties. This is how builtin elements work. 2) If it really needs to be set as a property, the page should ensure that the element is hydrated before React renders (by placing the element's script before Reacts, or making it a higher-level import in the bundle).


As an aside, I find it's easier to think about these kinds of problems when you think about what elements do, rather than specifically what custom elements do. How do you use ensure that the valueAsNumber property on inputs hydrates properly? Maybe internally React special-cases valueAsNumber (a search doesn't find anything), but more likely the user should not be using valueAsNumber in SSR and should be setting value instead. The solution is that the user should 99% of the time be using serializable attributes.

bahrus commented 3 years ago

I think it would be great if all web components did exactly what @matthewp is talking about -- I think maybe the web component community should adopt some checklist to give a "passing grade" on that basis. I suspect most do, as most are built using a high quality library, but it could certainly be something one of them missed. I agree that's not on React to fix or even work around really (except maybe mentioning in the documentation).

I imagine it would be better to reduce the number of promises/awaits floating out there, waiting for elements to register. Maybe not a big deal, but something to consider.

morewry commented 3 years ago

I keep seeing people say this, but it really doesn't make sense to me. Why is it "not React's job" as a DOM and web abstraction layer to provide adequate support for new DOM and web features?

bahrus commented 3 years ago

Oh, I most definitely do think it is their job, or at least I would like for them to think so. I believe option 3 would be the way to go, but it appears to be off the table. In the option 2 world, where they've failed their job ( :-) maybe that's too harsh ), we have to choose between two not so great options, I think. We either forcibly wait for each custom element to upgrade before passing most relevant things in, which could be costly in terms of promises awaiting, (maybe small, I don't know) and cause additional FOUC for developers who use their custom attributes for light and shadow DOM styling, or we risk developers encountering an unexpected bug, and having to write a little more code should they choose to use dynamic import. The latter option might be better, I don't know, if we can detect it right away, and if React could clearly and objectively :-) let the developer know what to do and why they need to do it.

I could be missing something, that's how I see the options at this point.

To be fair, there is a rather tricky, non intuitive and bizarre trick which @matthewp has alluded to, which I think needs more attention/awareness among the web component community, maybe a language fix to make it easier, so that props can be passed in ahead of time, before web components have upgraded. I myself failed to adequately account for it, I recently discovered, now that class fields are supported natively in browsers.

ConradSollitt commented 3 years ago

@matthewp Great demo on your CodePen, I didn't even know (or forgot) that attempting to set an attribute before the element is defined results in it not working. I'm used to making sure all my code runs customElements.whenDefined to avoid that issue but you demonstrate how it's easy to break.

All, The is one condition condition (likely edge case) I'm aware of when using Option 2. Basically if a Web Component defines only a getter and not a setter then both Preact and Vue 3 break, but the current version of React doesn't (at least React 16 and 17).

Condition that would cause an error if using Option 2 but will work with React 16 and 17

// Only getter is defined
get url() { return this.getAttribute('url'); }

Fix for Web Component Developers

// Define both getter and setter
get url() { return this.getAttribute('url'); }
set url(newValue) { this.setAttribute('url', newValue); }

In the last version of Preact I checked it used this exact code:

    !isSvg &&
    name in dom
) {
    dom[name] = value == null ? '' : value;

To handle the issue the code would need to look more like this (but optimized to avoid multiple calls to Object.getOwnPropertyDescriptor).

    !isSvg &&
    name in dom &&
    Object.getOwnPropertyDescriptor(Object.getPrototypeOf(dom), name).set !== undefined &&
    Object.getOwnPropertyDescriptor(Object.getPrototypeOf(dom), name).writable !== false
) {
    dom[name] = value == null ? '' : value;

I once opened an issue on Preact for this until I figured it out. It has a lot more details and talks about trade offs of different options in case anyone is interested: https://github.com/preactjs/preact/issues/2860

Personally I think React should use Option 2 and not Object.getOwnPropertyDescriptor. However if using Option 2 it might break some apps that work with React 16 and 17 if they use Web Components and for this reason I would simply recommend having a quick Note/Yellow section in the docs mentioned how to handle the issue.

I'll be happy to help create a clear CodePen example later if needed and prior to React 18 release I'll be testing it in detail with client-side Web Components (never used SSR for Web Components).

josepharhar commented 3 years ago

Thanks for the feedback everyone! This is very helpful!

@bahrus

if somehow React could detect that a developer is using dynamic import, and nevertheless trying to use standard JSX prop binding -- maybe it could just throw an error, or at the very least, during development, issue a stern console warning, with suggested syntax

I see - we could explicitly not support the case where you are trying to render JSX attributes onto a custom element which hasn’t been upgraded yet by doing nothing until the custom element has been upgraded - it does sound like this isn’t a case people are running into often… This line of thinking would suggest that we shouldn’t render any non-builtin HTML attributes when server rendering custom elements though.

@matthewp

Here's a codepen demonstrating that issue: https://codepen.io/matthewp/pen/rNmjoLW?editors=1010. You can fix it by uncommenting the defineProperty in the constructor

I agree this is a problem we should avoid. I’m not currently considering any solutions where we would actually set properties on a custom element before it gets upgraded. We can set HTML attributes before it is upgraded though - that is what preact currently does.

The custom element can handle properties possibly being set as either attributes or properties. This is how builtin elements work

Yes, this is what is needed in order to make what preact currently does fully work, since JSX attributes will be rendered as HTML attributes instead of properties when the custom element isn’t upgraded yet. However, the custom element may also need to be aware of the fact (in the case of preact) that you can end up with both an HTML attribute and an object property of the same name are set - if the element is server rendered or rendered before upgraded, then the custom element is upgraded, and then the JSX value changes and gets re-rendered, the old value will be left in the HTML attribute and the new value will be assigned to the object property.

That was kind of a mouthful, let me try to explain more practically. Here is what preact will do today with SSR, although it will do the same thing if you try to render before the custom element is upgraded:

Step State after step
renderToString of <my-custom-element myprop={'oldvalue'} /> gets parsed by the browser attribute:'oldvalue' property:undefined
hydration occurs, again with <my-custom-element myprop={'oldvalue'} /> attribute:'oldvalue' property:undefined
new render as <my-custom-element myprop={'newvalue'} /> attribute:'oldvalue' property:'newvalue'

The custom element shouldn’t try to use the attribute after 'newvalue' is assigned into the property because the attribute has 'oldvalue'. I’m not sure if this is a problem that custom elements are likely to run into. If this is never a problem, then perhaps the preact’s current behavior is acceptable. This is something that should go in our "checklist" of compatible custom element behavior.

@bahrus

We either forcibly wait for each custom element to upgrade before passing most relevant things in, which could be costly in terms of promises awaiting, (maybe small, I don't know) and cause additional FOUC for developers who use their custom attributes for light and shadow DOM styling, or we risk developers encountering an unexpected bug, and having to write a little more code should they choose to use dynamic import

This is a good point, and I think it suggests that we should go with the unexpected bug (preact’s current behavior) route because it enables developers to avoid FOUC and other potential performance issues.

To be fair, there is a rather tricky, non intuitive and bizarre trick which @matthewp has alluded to, which I think needs more attention/awareness among the web component community, maybe a language fix to make it easier, so that props can be passed in ahead of time, before web components have upgraded. I myself failed to adequately account for it, I recently discovered, now that class fields are supported natively in browsers.

This does sound cool, but I don’t see how we could leverage it in option 2 - we're never going to try to assign a JSX attribute to an object property before the custom element is upgraded, only HTML attributes (apologies if this isn't what you were alluding to).

@ConradSollitt

Condition that would cause an error if using Option 2 but will work with React 16 and 17

// Only getter is defined
get url() { return this.getAttribute('url'); }

I once opened an issue on Preact for this until I figured it out. It has a lot more details and talks about trade offs of different options in case anyone is interested: preactjs/preact#2860

However if using Option 2 it might break some apps that work with React 16 and 17 if they use Web Components and for this reason I would simply recommend having a quick Note/Yellow section in the docs mentioned how to handle the issue.

I'll be happy to help create a clear CodePen example later if needed and prior to React 18 release I'll be testing it in detail with client-side Web Components (never used SSR for Web Components).

It took me a couple minutes to wrap my head around this, but I understand - having a getter makes the 'url' in dom heuristic believe that it should use a property instead of an attribute, but if you don't have a setter, then your custom element wouldn't pay any attention to the property coming in and would effectively drop the value...

On one hand, it doesn't sound like too much to ask for custom elements to pay attention to this situation as something we can add to our "checklist" of compatible custom element behavior, but on the other hand, I wonder if we would seriously be breaking any existing usage of custom elements with this change...

ConradSollitt commented 3 years ago

@josepharhar Thanks for reviewing the issue and considering it. I like your idea of having a "checklist" of compatible custom element behavior. I wouldn't expect this to break many component libraries for React (if any); if it did break something it would be a quick fix on the authors part. I've authored some Web Components and that's how I found the issue (basically the Components worked with React at first but not Preact and Vue 3 until I made correction on my end. To be certain I went through the source code of all 3 projects until I could find the exact lines of code on how each library was handling it.

I can see this happing to some Web Component authors if they need/want a getter but typically define properties through HTML attributes. It takes extra code to keep properties and attributes in sync so based on my past needs I would only add properties if I expected to use them rather than always adding both getter and setter.

I just checked Shopify's new default development theme https://github.com/Shopify/dawn which uses a lot of custom elements and they didn't use a getter or setter anywhere in the code. I wouldn't expect developers to use this code with React but the fact that it's a major company/project using custom elements I can see other developers using the same style in the future especially if they are using the theme.

bahrus commented 3 years ago

Yes, @josepharhar, in the most imperfect world option 2 forces us into (why is JSX so ossified? (sigh)), I think Preact's solution is the better one. I have confirmed that there are some high quality web component libraries, such as those from SAP (UI5) which use attributes as key "state expressions" on which styling is based, so artificial delays in being able to set those attributes seems like a non-starter to me (but it seems React always takes whatever road is worse for "naturalized," imported DOM elements, so maybe I should lie about that :-) )?

It's possible an important point was missed in finding the third option attractive (or maybe I'm misunderstanding how SSR works) -- Most (maybe all?) web component libraries that have non-string properties, especially if they are object properties, do one of two things -- expect, or at least hope that the attribute value will be a JSON string, which in my mind is a great feature, or simply ignore it. Perhaps if React goes with the Option 2/Preact model, these libraries will need to add a little more validation logic to handle React's incompatibility-with-the-full-DOM-API limitations - for example, adding a try/ignore for JSON-based attribute binding. Little harm will therefore be done by setting an attribute value for one of these properties, especially if it can be set to something other than [Object object]. I.e. the damage done to naturalized, imported custom elements, is not primarily from incorrect use of attributes on the server-side, but rather from incorrect use of properties on the client side, by incorrectly assuming they are string attributes, and more importantly by not passing in the crucial object property. Or am I missing something? I know I'm making a bold statement with little SSR experience, so I'm guessing a little here.

And most web component libraries are built to handle loading properties asynchronously, so they don't "act" until they have enough information to act properly. And another thing -- the web community is gelling around a non-global attribute called "defer-hydration" meant specifically for SSR. So Option 2 / sub-option 3 would appear to kneecap that solution (hopefully not intentionally).

Preact seems to give developers more alternate routes to achieve their goals without punishing the user (avoid JSX property binding on the client-side when using dynamic import, maybe finding ways of JSON.stringifying the property on the server-side before setting the attribute (or does React make that impossible?, etc.) -- is there any way to use JSX binding on the server-side, but not the client?) More annoying busy work to work around issues is better than tying the hands of developers, and sacrificing performance/usability.

josepharhar commented 3 years ago

use attributes as key "state expressions" on which styling is based, so artificial delays in being able to set those attributes seems like a non-starter to me

Could you elaborate on "artificial delays"? Are you saying that we shouldn't wait for the custom element to be upgraded before throwing all JSX attributes into HTML/element attributes, especially when server rendering?

It's possible an important point was missed in finding the third option attractive (or maybe I'm misunderstanding how SSR works) -- Most (maybe all?) web component libraries that have non-string properties, especially if they are object properties, do one of two things -- expect, or at least hope that the attribute value will be a JSON string, which in my mind is a great feature, or simply ignore it. Perhaps if React goes with the Option 2/Preact model, these libraries will need to add a little more validation logic to handle React's incompatibility-with-the-full-DOM-API limitations - for example, adding a try/ignore for JSON-based attribute binding. Little harm will therefore be done by setting an attribute value for one of these properties, especially if it can be set to something other than [Object object]. I.e. the damage done to naturalized, imported custom elements, is not primarily from incorrect use of attributes on the server-side, but rather from incorrect use of properties on the client side, by incorrectly assuming they are string attributes, and more importantly by not passing in the crucial object property. Or am I missing something? I know I'm making a bold statement with little SSR experience, so I'm guessing a little here.

I agree that JSON.stringifying objects to strings for HTML attributes seems like a good addition to the preact behavior, and that custom elements should probably be able to handle that if they're interested in supporting rendering before the custom element is defined and server rendering.

And most web component libraries are built to handle loading properties asynchronously, so they don't "act" until they have enough information to act properly. And another thing -- the web community is gelling around a non-global attribute called "defer-hydration" meant specifically for SSR. So Option 2 / sub-option 3 would appear to kneecap that solution (hopefully not intentionally).

I'm not sure I understand - are you saying that if you have defer-hydration in your JSX, then preact won't apply it to an HTML attribute as desired...?

is there any way to use JSX binding on the server-side, but not the client?

Considering that react will emit warnings/errors in developer mode when the JSX attributes you're rendering during hydration don't match the HTML attributes set during server rendering, I don't think so...

Maybe if we had a way for react to allow you to emit whatever HTML you want when server rendering a custom element that you could opt into, then we could support this as well as other fun things like declarative shadowdom

bahrus commented 3 years ago

Yes, that's what I'm saying. I don't like blocking attributes from server-side rendering.

UI5 has css selectors that depend on custom attributes. For example:

[inner-input][inner-input-with-icon]{
...
}

They actually applied this to the native input element (much to some of the React team's displeasure, I'm sure, and which I actually share some of that concern, although they did use dashes, so likely no future conflicts).

The following is more classic, which I more heartily endorse, is also found with UI5:

:host([value-state="Success"]:not([readonly])) {

So to avoid FOUC, for both light children (and if they add support for declarative shadowDOM), allowing the JSX to set those attributes on the server seems really valuable.

I've verified in this case, the corresponding property name for "value-state" is "valueState" (so there is a mapping issue between attributes and the corresponding property name). So possibly with Option 2 / preact, you would need two binding rules in this case:

<ui5-combobox value-state={myState} valueState={myState}>

Not all web component libraries adhere to that naming convention as far as attributes / properties. So things could get a bit weirder in the case that they use the same name for the attribute and property. And web components that get a stamp of approval from the W3C, and become native-born DOM elements, most likely won't follow that naming convention. I have to wrap my brain around that specific scenario (especially when it is an object property). I don't think there's an issue there, other than the double binding problem discussed below.

Even if JSX emits two attributes on the server: value-state, and "valueState" -- note that when it comes to reading attributes, attributes are case insensitive, but dashes are significant, not sure about styling -- the second attribute wouldn't do any harm.

Since in this particular case, the UI5 combox property valueState is of type string, there probably isn't an issue here (except ui5-combobox might see two changes for every single actual change if it observes attribute changes). But if it were of type object, some libraries that expect it be a JSON attribute might throw/log an error, but could probably be adjusted to avoid that inconvenience, and just ignore it. If bundling is used, then by the time client side binding occurs, React would know it is a property, and pass it in correctly. I don't know what preact does with value-state={myState} on the client. If there happens to be a property named value-state, which seems really unlikely, it might do myUi5ComboBox['value-state'] = 'myState'. If not, I think it would continue to set the attribute value as states change (I guess), and set property valueState. So ui5-combobox could quite likely "react" twice for every state change, but that might not do much harm if there's filtering if values don't really change, and some form of debouncing. So that harm could be mitigated.

So the one big use case where things are problematic is with properties that are objects on the client, and that use dynamic import. So developers could be instructed to bypass JSX's binding in that case on the client side, but use JSX for server-side attribute binding, especially for styling, and make their components ignore malformed values passed in as attributes on the client (I guess). Which works for compound names with dashes in attributes, at least. There might be an issue with single syllable names (but I don't want to jump to conclusions).

Again, I'm not endorsing this state of affairs, but trying to find the most workable solution (and I might be missing something).

I think preact will handle defer-hydration just fine.

Bottom line, I think preact's behavior is a good step forward, but I will still be sighing impatiently (if anyone cares :-) ).

ConradSollitt commented 3 years ago

@robdodson Is there an overall goal of Web Components for React v18? @gaearon mentioned the site https://custom-elements-everywhere.com/

For example, is a perfect score with that site desirable? I came across that site a while ago during research and testing and was surprised by the React score - when testing React, Vue, and Preact only React worked for Components I initial developed. The main reason being that I targeted HTML attributes for an API and mostly used a single value attribute to pass JS objects for data binding and other features. I mentioned in an earlier comment that I ended up updating my Components to work with both Preact and Vue and had no problems with React.

@bahrus Have you found any major issues with React and Web Components? If so can you provide a high level overview (e.g.: Bullet Points) and libraries if they are published publicly? For example, do you use the SAP Components or other major Libraries and do they have any issues?

@sebmarkbage mentioned the data- attributes in an earlier comment. For me personally I have used these extensively for the past 10 years outside of React Development so I can see a consideration being something along the lines of:

if (!isSvg && name in element && !name.startsWith('data')) {
    element[name] = value == null ? '' : value;
} else {
    element.setAttribute(name, value)

NOTE - the above code is a simplified example.

@josepharhar Are you aware of any Web Component Libraries with React that run into issues. For example with https://lit.dev/ or any internal libraries at Google or on the Chromium team?

@ Anyone at FB Does anyone have apps that use both Web Components and React for the same app? If so, are there any challenges to be addressed? Personally I like developing with both technologies but unless proven wrong I feel that when working with React my need for Web Components is greatly reduced. Basically if using Web Components with React the I feel the Web Component should be very focused and generic (e.g.: a Color Picker); otherwise to me personally I prefer to use a React Component. And when I'm working with Web Components I currently view it as good for Vanilla JS apps (similar to jQuery development); or for a non-build SSR rendered app like how Shopify used it on there new default theme. For my day job I've used both on recent apps but used them seperately.

@ Anyone Ever use SSR Web Components? If so what technologies are you using and how it is relevant to this issue? I wasn't aware of SSR Web Components being used in real life until reading this issue, although seem very niche to me. If there is a good reason for using both at the same time with SSR it would be good to learn from.

bahrus commented 3 years ago

Hi @ConradSollitt

Based on the findings of @robdodson, I adopted Preact in a few projects. We built web component wrappers around api libraries like eCharts. We are a financial company, so we use lots of grids and charts, so the ability to send objects (not strings) into and out of components (via events) is rather critical for us. Being able to load the components asynchronously is also quite important, as we have not adopted SSR yet, and those components are really heavy, and Preact is really light, hence my obsession regarding that issue.

That's an interesting observation regarding issues with getters but no setters (all of which ought to be easily resolved with option 3, as guesswork isn't necessary! The beauty of stating intentions explicitly! Too bad JSX, controlled by a single company, seems to be stuck, incapable of evolving, while the web standards themselves, involving open coordination between multiple vendors, have been evolving at a breathtaking pace. I didn't think I would ever behold such a thing, but here we are! Okay, yeah, JSX introduced fragments (yawn), so there's that). Preact more or less behaved as advertised, meaning it worked. We did encounter an issue working with web components that used templates. There is an (undocumented, kind of ugly, last I checked) work-around for that issue.

I've briefly used React with simple, string-only properties/attributes web components (MWC), and it worked fine (no SSR). Again, no conflicts with @robdodson's findings. So probably not enough to comment on this as much as I have, point well-taken.

However, I can report that another group I work with used React with a datagrid web component, and, surprise, surprise, immediately faced the issue of not being able to pass the large dataset in, which doesn't leave a good first impression about the two working together. They knew I was passionate about web components, so they asked me what to do, and I pointed out @robdodson's suggested workaround, which worked for them. If it wasn't for that connection, I'm sure they would have quickly abandoned using web components. So my comments on documentation are grounded in (very limited) real-world experience. I'm quite disappointed React never even bothered to document the workaround in all this time. I recognize anti-competitive proprietary lock-in may appear in Facebook/React's short-sighted self-interest, but it is disappointing to see it in action so up close. At least that's what I attribute the very low priority this issue has been given, and what this is all about, really.

gaearon commented 3 years ago

What’s the workaround you want to see documented? I’m happy to take a PR!

I understand why you may see this as a desire for lock-in and I know you probably won’t take my word for it, but there’s no such intent. If it was obvious what the “right” solution is, in a way that works with the whole React feature set (including SSR), we would’ve implemented it years ago. We were hoping that with time, best practices would emerge, but as you can see from this thread there’s still so much fragmentation and disagreement, and almost resignation to not care about SSR. Which is ironic because people who are frustrated with React not moving forward on this issue often tend to be the same people who are frustrated with the client rendering model. Anyway, I’m getting into a rant, and rants aren’t a good discussion form. On a positive note, we’ve had a few more discussions with @josepharhar, and the proposal is getting more solid.