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

Checking `prop in elem` fails with deferred custom element definitions #678

Open
treshugart opened this Issue Apr 27, 2017 · 9 comments

Comments

Projects
None yet
5 participants
@treshugart

treshugart commented Apr 27, 2017

Reproduction: https://www.webpackbin.com/bins/-KikMSw9W7D7EHbuTdTX

Context: Skate is using Preact under the hood to render, so h is just a straight export of Preact's h in that bin. val is a function that ensures props are explicitly set all of the time. Another way to work around the check here would be to use a ref and imperatively set the prop.

I spoke with @developit and the proposed solution is to always set props for anything that isn't a string or number, while continuing the prop in elem check. However, I'm not sure the in check is still necessary after implementing something like this.

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Apr 27, 2017

Owner

It's also possible #492 might end up benefitting from a solution to this.

Owner

developit commented Apr 27, 2017

It's also possible #492 might end up benefitting from a solution to this.

@robdodson

This comment has been minimized.

Show comment
Hide comment
@robdodson

robdodson Jul 13, 2017

Resuming our twitter uber thread here :) cc @justinfagnani

In my head I was imagining that serializing to a JSON string when you can't find the property would be preferable because it supports both the lazy load scenario, and SSR.

I should also add I'm not strongly opposed to the idea of setting properties, I think either approach will work fine since a Custom Element author is going to need to account for either scenario (prop was set or JSON string was passed to attribute).

robdodson commented Jul 13, 2017

Resuming our twitter uber thread here :) cc @justinfagnani

In my head I was imagining that serializing to a JSON string when you can't find the property would be preferable because it supports both the lazy load scenario, and SSR.

I should also add I'm not strongly opposed to the idea of setting properties, I think either approach will work fine since a Custom Element author is going to need to account for either scenario (prop was set or JSON string was passed to attribute).

@justinfagnani

This comment has been minimized.

Show comment
Hide comment
@justinfagnani

justinfagnani Jul 13, 2017

Choosing attribute vs property based on the type of the value just seems wrong to me. Properties and attributes two different APIs, and there should be some actual control over which you invoke. What's wrong again with letting the developer choose based on the JSX attribute name?

justinfagnani commented Jul 13, 2017

Choosing attribute vs property based on the type of the value just seems wrong to me. Properties and attributes two different APIs, and there should be some actual control over which you invoke. What's wrong again with letting the developer choose based on the JSX attribute name?

@treshugart

This comment has been minimized.

Show comment
Hide comment
@treshugart

treshugart Jul 13, 2017

Choosing attribute vs property based on the type of the value just seems wrong to me.

I tend to agree. Maybe 100% props all of the time unless prefixed with aria- or data-. Escape hatch could be a prefix with attr- maybe? Just spitballin.

treshugart commented Jul 13, 2017

Choosing attribute vs property based on the type of the value just seems wrong to me.

I tend to agree. Maybe 100% props all of the time unless prefixed with aria- or data-. Escape hatch could be a prefix with attr- maybe? Just spitballin.

@robdodson

This comment has been minimized.

Show comment
Hide comment
@robdodson

robdodson Jul 13, 2017

Oh I agree that long term it might be beneficial for there to be syntax which supports setting attr or prop. I wasn't actually trying to get into that discussion here, but rather was curious how Preact should handle an undefined object or array given what JSX currently supports. At the moment if the property is undefined, Preact will set an object to <x-foo obj="[object Object"]> and an array to <x-foo arr="1,2,3"> (note the Array is missing square brackets). Initially I thought that it should JSON serialize stuff but am in agreement now that it would be better for it to just set complex objects as properties on the element. Custom Element authors can use a lazy properties approach to handle this.

The last thing I was wondering about was how to deal with SSR. Should an object or array get serialized in that scenario since there's no ability to set a property. After discussing this with @justinfagnani and @developit I think it's best if Preact just does nothing here. Like, don't even try to set the attribute to anything. My reasoning is that I couldn't think of a use case where this would actually be beneficial to the Custom Element. The element can't boot up server side, and even if it could, it can't render shadow DOM so what good is it going to do to pass it a complex object serialized to an attribute? Also, if you were able to somehow upgrade the element server side then you could just set the property on it.

To make this more explicit I tried to write up a list of expected behaviors. Curious what you all think?

Summary of expected behavior

  • If an element has a defined property, Preact will use it.
  • If an element has an undefined property, and Preact is trying to pass it a string/number/boolean, it will use an attribute.
  • If an element has an undefined property, and Preact is trying to pass it an object/array it will set it as a property. (related to #511 )
  • If the element is being rendered on the server, and Preact is trying to pass it a string/number/boolean, it will use an attribute.
  • If the element is being rendered on the server, and Preact is trying to pass it a object/array, it will not do anything.

robdodson commented Jul 13, 2017

Oh I agree that long term it might be beneficial for there to be syntax which supports setting attr or prop. I wasn't actually trying to get into that discussion here, but rather was curious how Preact should handle an undefined object or array given what JSX currently supports. At the moment if the property is undefined, Preact will set an object to <x-foo obj="[object Object"]> and an array to <x-foo arr="1,2,3"> (note the Array is missing square brackets). Initially I thought that it should JSON serialize stuff but am in agreement now that it would be better for it to just set complex objects as properties on the element. Custom Element authors can use a lazy properties approach to handle this.

The last thing I was wondering about was how to deal with SSR. Should an object or array get serialized in that scenario since there's no ability to set a property. After discussing this with @justinfagnani and @developit I think it's best if Preact just does nothing here. Like, don't even try to set the attribute to anything. My reasoning is that I couldn't think of a use case where this would actually be beneficial to the Custom Element. The element can't boot up server side, and even if it could, it can't render shadow DOM so what good is it going to do to pass it a complex object serialized to an attribute? Also, if you were able to somehow upgrade the element server side then you could just set the property on it.

To make this more explicit I tried to write up a list of expected behaviors. Curious what you all think?

Summary of expected behavior

  • If an element has a defined property, Preact will use it.
  • If an element has an undefined property, and Preact is trying to pass it a string/number/boolean, it will use an attribute.
  • If an element has an undefined property, and Preact is trying to pass it an object/array it will set it as a property. (related to #511 )
  • If the element is being rendered on the server, and Preact is trying to pass it a string/number/boolean, it will use an attribute.
  • If the element is being rendered on the server, and Preact is trying to pass it a object/array, it will not do anything.
@majo44

This comment has been minimized.

Show comment
Hide comment
@majo44

majo44 Nov 15, 2017

@robdodson
I'm not sure about this point.

If the element is being rendered on the server, and Preact is trying to pass it a object/array, it will not do anything

I'm working on some project where whole view is created as a tree of web components. We are prerendering them on server side. On side of markup we are sending to the client also the state of application (objects/arrays) which we are populating on client side to the components tree.

We know that in the components we have to remember that without some data we should not rerender the component, wait until we have required data. But such solution works fine, and on client side during rehydirgation we are not creating even single new DOM node, we are reusing whole prerendered dom tree.

We are using simple rule, primitives we are always sets as attributes, objects/arrays/functions we are setting as properties.

Current tech stack:

majo44 commented Nov 15, 2017

@robdodson
I'm not sure about this point.

If the element is being rendered on the server, and Preact is trying to pass it a object/array, it will not do anything

I'm working on some project where whole view is created as a tree of web components. We are prerendering them on server side. On side of markup we are sending to the client also the state of application (objects/arrays) which we are populating on client side to the components tree.

We know that in the components we have to remember that without some data we should not rerender the component, wait until we have required data. But such solution works fine, and on client side during rehydirgation we are not creating even single new DOM node, we are reusing whole prerendered dom tree.

We are using simple rule, primitives we are always sets as attributes, objects/arrays/functions we are setting as properties.

Current tech stack:

@robdodson

This comment has been minimized.

Show comment
Hide comment
@robdodson

robdodson Nov 15, 2017

Sorry, I'm having a hard time understanding your comment. Are you saying that on the server you're using domino to allow custom elements to run their lifecycle callbacks so you can give them data as properties and they can stamp out their children?

robdodson commented Nov 15, 2017

Sorry, I'm having a hard time understanding your comment. Are you saying that on the server you're using domino to allow custom elements to run their lifecycle callbacks so you can give them data as properties and they can stamp out their children?

@majo44

This comment has been minimized.

Show comment
Hide comment
@majo44

majo44 Nov 15, 2017

@robdodson

For as is transparent where we are rendering the component, on server (domino) or client. Components works exactly in same way on both sides, have same live cycle and expect same data. We are even do not have conditional code like isServer/Client. So yes we are building the component dom sub tree (attached to pseudo shadowDom) on server, serializing it by document.body.innerHTML and sending it to the client.

On client side after loading the html, inlined state and javascript code, components starts live cycle on client side. If component have a attached pseudo shadow dom (sub tree) we are reusing it whole, if not we are rebuilding it.

majo44 commented Nov 15, 2017

@robdodson

For as is transparent where we are rendering the component, on server (domino) or client. Components works exactly in same way on both sides, have same live cycle and expect same data. We are even do not have conditional code like isServer/Client. So yes we are building the component dom sub tree (attached to pseudo shadowDom) on server, serializing it by document.body.innerHTML and sending it to the client.

On client side after loading the html, inlined state and javascript code, components starts live cycle on client side. If component have a attached pseudo shadow dom (sub tree) we are reusing it whole, if not we are rebuilding it.

@robdodson

This comment has been minimized.

Show comment
Hide comment
@robdodson

robdodson Nov 20, 2017

I see. So I guess it really depends on whether or not Preact is being paired with a functioning DOM abstraction on the server side that can properly trigger all of the lifecycle callbacks for custom elements. In that case it should (I guess?) be able to use the same behavior as on the client.

If the element is being rendered on the server, and Preact is trying to pass it a object/array, it will not do anything.

I wrote this comment specifically imagining a situation where there isn't a DOM abstraction but instead the whole app is trying to be rendered to a big string or something entirely in Node. I have to admit my knowledge of SSR is pretty limited so if this doesn't seem like the right behavior I'm more than happy to say we should change it :)

robdodson commented Nov 20, 2017

I see. So I guess it really depends on whether or not Preact is being paired with a functioning DOM abstraction on the server side that can properly trigger all of the lifecycle callbacks for custom elements. In that case it should (I guess?) be able to use the same behavior as on the client.

If the element is being rendered on the server, and Preact is trying to pass it a object/array, it will not do anything.

I wrote this comment specifically imagining a situation where there isn't a DOM abstraction but instead the whole app is trying to be rendered to a big string or something entirely in Node. I have to admit my knowledge of SSR is pretty limited so if this doesn't seem like the right behavior I'm more than happy to say we should change it :)

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