New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add create-custom-element-type RFC #15

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@robdodson

robdodson commented Jan 22, 2018

RFC to address facebook/react#11347.

The changes in this RFC specifically address how React passes data to custom
elements and listens for their DOM events.

This RFC has two parts:

  1. React would change its current behavior such that when it passes data to a
    custom element, it uses JavaScript properties instead of calling
    setAttribute.

  2. React would add a new API, ReactDOM.createCustomElementType() which would
    create a wrapper React component that knows how to map properties and event
    handlers back to a custom element.

cc @developit @treshugart @effulgentsia @justinfagnani

@facebook-github-bot

This comment has been minimized.

Collaborator

facebook-github-bot commented Jan 22, 2018

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up 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 the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

it's easiest to convert camel cased properties to lowercase attributes, without
a delimiter, when calling `ReactDOMServer.renderToString()`.

## 2. ReactDOM.createCustomElementType()

This comment has been minimized.

@streamich

streamich Jan 23, 2018

Why not call it just createCustomElement?

This comment has been minimized.

@robdodson

robdodson Jan 23, 2018

I don't have a strong opinion. This was the name suggested by @gaearon when we were first kicking around the idea. facebook/react#11347 (comment)

This comment has been minimized.

@robdodson

robdodson Jan 23, 2018

On second thought, I think it would be a little misleading to call it createCustomElement since what it returns is not actually a custom element, but instead a React component that wraps a custom element.

This comment has been minimized.

@PinkaminaDianePie

PinkaminaDianePie Feb 9, 2018

maybe createCustomElementWrapper in that case?

This comment has been minimized.

@streamich

streamich Feb 22, 2018

.createCustomElementTypeWrapper()?

@facebook-github-bot

This comment has been minimized.

Collaborator

facebook-github-bot commented Jan 27, 2018

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

}
return (
<XFoo longName={name}

This comment has been minimized.

@treshugart

treshugart Feb 2, 2018

I'm having some ideas now that I've seen this part specifically. The 95% use-case is probably that you're going to be able to have access to the custom element constructor where you're writing the React code. The rest of the time, you may want deferred upgrades.

Part 1 - you have the custom element constructor

If you have the custom element constructor, you actually don't need to define a wrapper or any information about it. You have the properties it defines as well as a way to diff and construct it if it needs to be patched.

In this scenario, React could internally just check if it's a custom element constructor and set props if defined, or attributes otherwise. They don't need the tag name, either.

I PR'd this to Preact awhile back for reference: developit/preact#715.

SSR-story

This assumes you have server-side support for whatever DOM features would be utilised (JSDOM / Undom / Domino, etc). If this is not the case, then it should fallback to Part 2.

Until there's support for declarative shadow DOM (whatwg/dom#510) custom elements that have shadow DOM would render the host + light DOM as it normally would with out shadow DOM (no special APIs to mention here, just the standard childNodes stuff). This requires JavaScript on the client to initialise and rehydrate the custom elements, but this is no different that what Part 2 is offering on the client.

Due to the tight coupling this part has with the DOM, it is assumed that it would be implemented as part of ReactDOM, as opposed to React.

Part 2 - deferred upgrades (no constructor)

You do what you're proposing in this RFC.

This comment has been minimized.

@thysultan

thysultan Feb 3, 2018

In the case of "Part 1" how would server-side rendering work?

This comment has been minimized.

@treshugart

treshugart Feb 3, 2018

You'd have to have JSDOM support or another server side implementation support, but it would render out to host + light DOM and then rehydrate on the client. I was making the assumption this would be implemented in react-dom, if possible. I'll update.

This comment has been minimized.

@robdodson

robdodson Feb 22, 2018

@treshugart hm I'm confused by some of your comments.

This comment has been minimized.

@robdodson

robdodson Feb 22, 2018

ugh, hate the new github behavior where shift-enter submits a comment instead of inserting a newline. Continuing on from above...

In your comment you start off by talking about Part 1 and using the element constructor. But Part 1 of my proposal assumed someone was just using the tag name <x-foo>. So you're commenting on a somewhat different use case.

This comment has been minimized.

@treshugart

treshugart Feb 26, 2018

@robdodson

So you're commenting on a somewhat different use case.

Sure, but what I'm saying is that most of the time you have access to the constructor; this is the happy path and I think it should be the primary consideration by this RFC. I believe itt enables better ergonomics because you can infer all your information from the constructor. Manual configuration is necessary when you don't have it, but it might be worth considering making this secondary.

There would be no attempt by the custom element to render its shadow dom content server side. I'm assuming that would all boot-up client side.

Ok, that's fair. Looking back I think this would naturally happen anyways if using react-dom and it was being run under a node-dom env. Let's not worry about this.

This comment has been minimized.

@robdodson

robdodson Feb 27, 2018

@treshugart Are you suggesting folks would never use their custom element tag name and would instead always use the constructor in their render functions? That's not actually something I would want to do tbh.

This comment has been minimized.

@treshugart

treshugart Mar 2, 2018

Ok, our ideals here are quite divergent. I was hopeful that using a custom element in React could be the same way you'd use a React component. I almost never use custom element tag names and it's been quite liberating. Sure in HTML you do, but this isn't HTML.

This comment has been minimized.

@robdodson

robdodson Mar 2, 2018

I think I'm concerned that if you're using the constructor, it may be tough to distinguish a custom element from a React component. Because they are different and may have subtle differences in their behavior, it seems like if you want something to be indistinguishable from a React component it may be better to use the ReactDOM.createCustomElementType() API to wrap the custom element in an actual React component.

This comment has been minimized.

@treshugart

treshugart Mar 2, 2018

Yeah, that's fair. I'm 100% happy with this RFC as is and am super stoked you're doing it. I really hope this gets in.

As an aside, and in terms of ergonomics, I think it would be a nice addition to eventually add support for constructors because it clean up the API, essentially making the usage of a custom element just as simple using a React component. The fact that two components can have inconsistencies isn't a WC / React thing, it's a component author thing and I think that's an orthogonal problem.

@jthoms1

This comment has been minimized.

jthoms1 commented Mar 15, 2018

I believe that this rfc would accommodate the needs of StencilJS. We would like to see this move forward because we will want to use it to bring Ionic Components to the React ecosystem.

@sophiebits

This comment has been minimized.

Member

sophiebits commented May 18, 2018

Does anyone use SSR in conjunction with WC? Or are there plans to? If there are I would heavily lean towards attributes by default because doing so allows React SSR to behave the same way as React on the client out of the box.

@robdodson

This comment has been minimized.

robdodson commented May 18, 2018

Does anyone use SSR in conjunction with WC? Or are there plans to?

Not really. The hard part is figuring out how to represent Shadow DOM (and the distribution that it does) in some form that doesn't require JavaScript. There has been a lot of discussion on this topic and some interesting experiments, but it's definitely not mainstream to SSR them.

I would heavily lean towards attributes by default

If React needs to pass an object or array to the element, what would happen in this case?

@sophiebits

This comment has been minimized.

Member

sophiebits commented May 18, 2018

If React needs to pass an object or array to the element, what would happen in this case?

I was thinking that if that were the case you'd have to write the custom attribute/property mapping.

@sophiebits

This comment has been minimized.

Member

sophiebits commented May 18, 2018

I'd like to avoid getting stuck in a situation where SSR + WC is a thing but it's abnormally difficult in React due to past decisions of ours. If that's vanishingly unlikely to happen then it's less of a concern.

@dantman

This comment has been minimized.

dantman commented May 18, 2018

You can't use refs to set properties in SSR and in SSR it's expected that all you can do is set attributes.

From the perspective of SSR a div and a custom-element, aren't that different. I don't see why someone happily making use of a few WebComponents in their project should expect that they can't start using SSR because of the WebComponents they're making use of.

@sophiebits

This comment has been minimized.

Member

sophiebits commented May 18, 2018

The pitfall is if we make React use properties for WC, then you turn on React SSR, your app won't behave properly if those WC don't treat attributes and properties identically.

@dantman

This comment has been minimized.

dantman commented May 18, 2018

The pitfall is if we make React use properties for WC, then you turn on React SSR, your app won't behave properly if those WC don't treat attributes and properties identically.

Sure, but if a WebComponent doesn't have attribute<->property mapping then you inherently cannot use SSR in any situation. It's not so much a pitfall as an expected platform limitation that is the fault of the WebComponent implementation. And it doesn't make sense to punish those using the majority of well designed WebComponent implementations that do implement attribute<->property mapping because there are a smaller subset of WebComponents that don't behave properly.

@robdodson

This comment has been minimized.

robdodson commented May 18, 2018

Is React aware of when it's doing SSR versus running on the client? In the RFC I had a suggestion that if ReactDOMServer.renderToString() is running it could always use attributes. Are there other ways to do SSR in React other than that API?

The pitfall is if we make React use properties for WC, then you turn on React SSR, your app won't behave properly if those WC don't treat attributes and properties identically.

I think most custom elements that take arrays or objects are doing so in order to render content into their shadow roots. But that wouldn't work in an SSR situation anyway, so those components would probably have to be booted up on the client regardless. From my perspective, if I have an entirely client-side app that uses React and web components, it might be annoying to have to write a lot of wrappers just to make it work.

@sophiebits

This comment has been minimized.

Member

sophiebits commented May 18, 2018

Is React aware of when it's doing SSR versus running on the client?

Certainly we are. (In fact it's two mostly-distinct codebases.) But as much as possible we'd like your app to work perfectly with SSR if it works on the client. If we use attributes on the server and properties on the client that creates a pitfall where the two modes might do totally different things, which is a good way to introduce bugs.

@mortenson

This comment has been minimized.

mortenson commented May 18, 2018

@sophiebits

Does anyone use SSR in conjunction with WC?

Yes, there's @skatejs/ssr and StencilJS has something as well.

@robdodson

This comment has been minimized.

robdodson commented May 19, 2018

Yes, there's @skatejs/ssr and StencilJS has something as well.

@treshugart and @adamdbradley for their thoughts.

Adam - does the stencil SSR middleware allow the use of shadow dom?

@robdodson

This comment has been minimized.

robdodson commented May 19, 2018

@sophiebits What's the existing SSR behavior in React for something like tabindex? If I do <div tabIndex="{foo}" />, will it use an attribute on the server and then switch to using the tabIndex property on the client if I update foo?

My (probably naïve) suggestion would be to match whatever behavior React uses in this situation.

I recognize the concern around not knowing if the property is supported, but it's equally possible for someone to write a custom element that doesn't support attributes. If the author doesn't add the attribute to their observedAttributes array and check for changes in attributeChangedCallback, it won't work. This is why we encourage devs to just have attribute<->property parity because it makes the component easier to reason about. Libraries for authoring web components typically add this parity by default.

edit: I'm basically just repeating @dantman's points, so +1 to his comments 😁

@sophiebits

This comment has been minimized.

Member

sophiebits commented May 20, 2018

Looks like tabIndex calls .setAttribute('tabindex', ...) on the client for both HTML and SVG.

@robdodson

This comment has been minimized.

robdodson commented May 20, 2018

Looks like tabIndex calls .setAttribute('tabindex', ...) on the client for both HTML and SVG.

oh interesting. Is it the case for any HTML attribute/property pair that if it was first rendered with SSR it will always continue to use the attribute and never the property? Or are there instances where it uses the attribute for the server rendered string, but switches to the property once the client boots up?

@sophiebits

This comment has been minimized.

Member

sophiebits commented May 20, 2018

No, we always use the tabindex attribute regardless of if the initial render was SSR.

@robdodson

This comment has been minimized.

robdodson commented May 20, 2018

oh, sorry, I think my question was a bit unclear. I was wondering if there was any precedent where React uses an attribute on the server and then switches to using the property on the client. Maybe the behavior for checked would be an example of doing that?

@robdodson

This comment has been minimized.

robdodson commented May 20, 2018

sorry for the double post, something else just occurred to me.

If I have a property, bar, that accepts primitive data like a string or number, and I do:

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

And I go to SSR that, will it just return <x-foo />?

One possibly nice aspect of using attributes on the server and properties on the client would be if my element had attribute<->property parity then I could do:

<x-foo bar={baz}>

and that should "just work" regardless of if I'm on the client or server. Whereas the ref example seems like it might break?

@dantman

This comment has been minimized.

dantman commented May 20, 2018

If I have a property, bar, that accepts primitive data like a string or number, and I do:

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

And I go to SSR that, will it just return ?

Yes, components are not mounted on the server. Any properties set by ref or by React (instead of being mapped to attributes) must be set on the client during hydration. They can't be set by the server.

@adamdbradley

This comment has been minimized.

adamdbradley commented May 21, 2018

@robdodson At this time no, Stencil does not use shadow dom with SSR.

Does anyone use SSR in conjunction with WC?

I'd like to clarify that custom elements and the use of shadow dom are two different things for stencil. All components use custom elements, but shadow dom is an opt-in feature (the slot API is polyfilled if slot is used w/out shadow dom). However, if I'm following this discussion the use of shadow dom shouldn't matter either way, it's more about passing data to custom elements.

As far as passing data during SSR, it's all being pass as through attributes, which are then translated to their properties on the SSR. When the html hydrates client side it's essentially doing the same thing. Default is the attribute some-name mapped to someName property. Currently it uses jsdom w/ some custom code to hydrate the custom elements, with the plan to move to puppeteer soon.

@effulgentsia

This comment has been minimized.

effulgentsia commented May 22, 2018

Does anyone use SSR in conjunction with WC?

Is this asking whether there are cases where WebComponents are used within projects that do some SSR? Or whether the WCs themselves need to have their attributes SSR'd? For the former, I think certainly there must be. For the latter, I can see it being useful to SSR HTML-defined attributes (tab-index, aria-*, class, etc.), but what would be the use-case of SSRing custom attributes? Those require the WC's JS code to run to make use of them anyway, so why not always leave them out of the SSR rendering and hydrate them as properties client-side?

@effulgentsia

This comment has been minimized.

effulgentsia commented May 22, 2018

what would be the use-case of SSRing custom attributes?

Actually, thinking about this more, are there cases where a WC is implemented in a way that requires a custom attribute to already be defined at construction time? In which case, would client-side hydration of the property occur too late? Or, would React be able to reliably hydrate the properties prior to the WC's constructor running?

@dantman

This comment has been minimized.

dantman commented May 22, 2018

For the latter, I can see it being useful to SSR HTML-defined attributes (tab-index, aria-*, class, etc.), but what would be the use-case of SSRing custom attributes? Those require the WC's JS code to run to make use of them anyway, so why not always leave them out of the SSR rendering and hydrate them as properties client-side?

They require the WC JS, but do not require the React JS to function if the data can be expressed in simple attributes.

It's not the optimal use, but remember that React can be used server side to render traditional html pages that are not hydrated on the client. And WebComponents still work in this context, i.e. to my knowledge you can make a traditional <form> out of <paper-input> elements. And while you of course need to load Polymer/polymer-input, you do not have to hydrate React.

@sophiebits

This comment has been minimized.

Member

sophiebits commented May 22, 2018

If the consensus is that all "responsible" CE/WC implementations treat properties and attributes exactly the same when assigned a primitive value (and the name/capitalization always matches exactly) then I guess I am open to defaulting to properties. It may still be surprising that passing an object works on the client and not on the server though.

@effulgentsia

This comment has been minimized.

effulgentsia commented May 22, 2018

It may still be surprising that passing an object works on the client and not on the server though.

Can we make it work on the server? Can the SSR logic be that for object values to call JSON.stringify()? That would be consistent with Polymer's approach to WC. As it is, per HTML spec, SSR logic needs different serialization logic for Booleans, so if it already needs to check type, why not also check for object?

@robdodson

This comment has been minimized.

robdodson commented May 23, 2018

Can we make it work on the server?

It may depend on the DOM implementation being used for SSR. As @adamdbradley mentioned, if you are using custom elements without shadow dom, then you can run it all through something like puppeteer which would properly trigger the lifecycle callbacks for custom elements, and handle setting properties.

Can the SSR logic be that for object values to call JSON.stringify()?

One issue to be aware of is that you lose object identity, so if one of the values I was trying to pass was a reference to another object or a DOM node, that would then be broken by the stringify step.

Usually when I'm passing a custom element an object or an array I'm expecting it to use that to render more children. If it can't actually boot the custom element up on the server (depending on how SSR is done) then it might be of limited value. But I don't personally have a strong opinion either way.

@joeldenning

This comment has been minimized.

joeldenning commented Oct 11, 2018

I might be missing something, but it seems that every react prop for createCustomElementType would be passed straight through as an element property? Is there a way for createCustomElementType to specify that a React prop should be passed only as an element attribute, and not as an element property? I ask not because that is a use case that is common, but just out of curiosity. Since the RFC already is proposing a brand new thing that has the flexibility to support attributes and properties, I think it might be good to make it possible to do any combination of attributes and properties.

@nickcolley

This comment has been minimized.

nickcolley commented Nov 7, 2018

Does anyone use SSR in conjunction with WC? Or are there plans to?

Not really. The hard part is figuring out how to represent Shadow DOM (and the distribution that it does) in some form that doesn't require JavaScript. There has been a lot of discussion on this topic and some interesting experiments, but it's definitely not mainstream to SSR them.

One of the best things about Server Side React is that if JavaScript fails you can serve a good fallback experience for users if you are following progressive enhancement.

A progressive web component looks like the following:

<custom-autocomplete>
  <select>
    <option>
    <option>
    <option>
  </select>
</custom-autocomplete>

Here if JavaScript failed we can fallback to a select input for the user, this is the same way the the datalist element works.

It would be a awesome if any decision around this proposal would make it easier to do progressive enhancement with Web Components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment