Support for attributes, type-extension CEs, and those loaded by HTML import #60

rnicholus · 2016-12-26T04:58:23Z

...I also added an "engines" property to package.json to reflect the earliest version of Node.js required to run the unit test suite, which appears to be 6+. No changes I made here influenced the required version of Node to run the tests, this was a pre-existing requirement. The "engines" property provides documentation of this requirement if nothing else.

Note: There are no breaking changes in this PR, only additions. The updated version number reflects this.

Background

I am personally a fan of HTML imports. While I understand this portion of the Web Components specification is controversial in some circles, I love the way this portion of the spec provides for standardized self-contained modularization of HTML, JS, and CSS for web components. I can easily define the HTML, CSS, and JS for my Web Component in appropriate files and tie them all together in a way that is transparent to the user, which simply imports the root HTML file using a single HTML import. While this certainly will increase the number of HTTP requests, HTTP/2 (supported in all modern browsers) mitigates this with connection multiplexing.

I tend to use HTML imports often when I write my own web components. A couple OSS WCs I wrote that utilize HTML imports include ajax-form and file-input. But this is not an uncommon pattern, so I quickly ran into trouble when attempting to easily integrate WCs into my React-based projects. For the most part, webcomponents/react-integration makes this possible for WCs where the constructor function is easily accessible, but more work was required to properly Reactify WCs that relied on HTML imports. My work to support these types of WCs includes the commits in this pull-request as well as a Webpack loader I wrote that provides easy access to the "root" URL of an HTML-importable WC.

Changes in this PR

My pull request provides support for the following:

Associating attributes with custom elements.
Reactifying a custom element given only its tag name.
Reactifying all type-extension custom elements.

The remainder of this PR description goes into more detail regarding the above three items.

Associating attributes with custom elements

In a perfect world, all element properties would be automatically reflected as attributes where appropriate. But there are a number of custom elements in existence that do not properly update attributes when property changes occur. I have come across many of these myself, which is what prompted some of the changes in this pull request. Admittedly, I'm also the author of a few such CEs. While the long-term goal is to "fix" these CEs such that they sync property changes to attributes whenever prudent, it is desirable to allow direct access to attributes at least until these CEs can be updated (if possible).

My pull request looks for properties that start with attr-, and passes the associated value directly to the CE as an attribute. For this type of property, object/array values are also stringified before setting the element attribute. There are corresponding unit tests to verify this new behavior.

Reactifying a custom element given only its tag name

This is especially important for web components that are loaded via HTML import. In those instances, the constructor can be accessed, but the process may not be intuitive. Most importantly, the CE may not be fully "created" at the time the React component wrapper is rendered. In this case, it is far easier to simply pass the tag-name. This cuts out the code that uses the CE constructor to determine the tag-name, which itself expects the CE to be fully created at that point.

You can see the changes I made to the index file along with a number of new unit tests. I also took care to update the Typescript definitions file to reflect the fact that either a CE constructor function or a tag-name (string) may be passed as the first parameter to the exported function.

Reactifying all type-extension custom elements

This one was pretty simple to address. If a property named is is passed to the reactified CE, ensure this value is set on the first render of the element. Once the element has been added to the DOM, specifying the is property doesn't seem to have any affect. This is not a React-specific issue, though rendering the CE initially without the is property produces the same problem.

I've also updated the README.md file to include documentation for all newly supported features.

… constructor or a string/tag-name)

treshugart · 2016-12-31T02:24:52Z

This is awesome @rnicholus, thank you! I've had limited availability, so hopefully I'll get to reviewing this soon. Sorry for the delay :(

rnicholus · 2016-12-31T02:31:33Z

No rush. I know there's a lot to digest here. Let me know if I can assist in any other way.

Aside: While this library with my changes + the Webpack loader addresses most of the issues I had while trying to integrate web components into a React app, one problem I haven't yet solved is how to get HTML imports and ES6 imports to play nicely. How can we de-dupe across both of these mechanisms? I've been giving that some thought, but have not come up with a good answer yet. In the meantime, I believe this is a step in the right direction.

treshugart · 2016-12-31T02:34:41Z

Re the aside, you might be able to solve it with a Webpack plugin that checks hashed content. Not sure, though, I haven't given it much thought either.

rnicholus · 2016-12-31T02:44:33Z

My fear is that the two just won't be able to work together. It really has to be one or the other in terms of de-duping. I could update my Webpack plug-in to rip out all the <script src=...> in the WC's HTML files and instead feed those through Webpack. This would mean they are included alongside all other JS resources in the generated bundle(s). I'm not 100% sure that will actually solve the problem though, and in some cases the <template> tags need to be present in the DOM before the corresponding WC JS is executed.

At this point, I'm not worrying too much about loading duplicate resources. In many cases, this probably won't be a big deal, until a project mixes a great number of HTML-imported WCs and ES6 imported code/WCs.

treshugart · 2017-01-09T01:59:57Z

README.md

+If the underlying web component you intend to Reactify requires some properties to be set directly on the element as attributes, include an `attr-` prefix on the property name. For example:
+
+```jsx
+<MyComponent attr-data-fo='bar' />


What if it used something like attributes={{ foo: 'bar' }}?

Seems like a reasonable alternative.

I've also been playing around with a similar pattern in a separate lib but added support for more special props https://github.com/skatejs/dom-diff#attributes.

treshugart · 2017-01-09T02:01:41Z

README.md

+
+```jsx
+render() {
+  const Form = reactify('form')


My first thought would be to do reactify('ajax-form') here instead, and not have to specify is on the component.

Will that work? That will result in the library rendering an element named ajax-form, but this isn't a valid tag name as far as I can tell. It's a form element.

I commented in the particular parts of the code that could handle this. Essentially, you'd try and look up the component with customElements.get(name) and then construct it using the constructor. Underneath the hood you could tell React to use the real tagName and set the is attribute.

I'm unsure how this will work, though. The v1 CE polyfill doesn't support customised built-ins, I don't think. I also don't think React will call createElement() with the correct arguments in order for them to work, even if it did (or the browser natively supported them).

I commented in the particular parts of the code that could handle this.

I'll take some time to look over those comments ASAP.

Essentially, you'd try and look up the component with customElements.get(name) and then construct it using the constructor

Most of this PR is driven to better support CEs loaded using HTML imports. During development, I spent quite a bit of time integrating my own CEs as well as a few other CEs that are loaded via HTML import (some of these are mentioned the docs I contributed). When HTML imports are involved, there is no guarantee that the CE constructor will be available by the time reactify is called. In that case, it seemed safer to rely on the tag name and allow the rendered element to be upgraded at some time after the element specification was loaded by the HTML import. This timing issue was what drove most of the changes in this PR.

I also don't think React will call createElement() with the correct arguments in order for them to work

My testing seemed to suggest that, as long as the is property is included on the native element as part of its first render, the customized built-in works as expected. But if is is added later, the element is never upgraded. That is why I included the change at https://github.com/webcomponents/react-integration/pull/60/files#diff-1fdf421c05c1140f6d71444ea2b27638R81, to ensure a customized built-in element is rendered correctly.

The v1 CE polyfill doesn't support customised built-ins, I don't think

That very well may be true. The v1 CE polyfill is fairly new to me. I have only recently started using it in new WC projects. But if this is missing, it seems this support will need to be added, since the is property used to mark an extended native element is still part of the spec as far as I know. Granted, all of my testing was with v0 polyfills.

+1 for automatically adding the is="" attribute without each instance having to put it on there. The v1 CE polyfill may soon have support for customized built-in, pending webcomponents/custom-elements#42.

Once skatejs-react-integration has support for customized builtins (when this pr is merged), myself and the company I work for are interested in using this. Love the work you're doing here!

treshugart

I didn't see a test for testing customised built-ins, but I'm not sure we could do that since I don't think the polyfill supports it yet.

treshugart · 2017-01-09T02:04:04Z

src/index.js

+
+  let CustomElementConstructor = CustomElementOrTagName
+  if (typeof CustomElementOrTagName === 'string') {
+    CustomElementConstructor = document.createElement(CustomElementOrTagName).constructor


You could use customElements.get(CustomElementOrTagName). This would have the added benefit of solving https://github.com/webcomponents/react-integration/pull/60/files#r95096012, I think.

This will return undefined if the element hasn't been imported yet.

treshugart · 2017-01-09T02:05:58Z

src/index.js

+  }
+
+  const tagName = typeof CustomElementOrTagName === 'string'
+      ? CustomElementOrTagName


You wouldn't need this part if looking up via customElemenst.get() but you would need to use the is attribute if it exists and fallback to tagName if not.

treshugart · 2017-01-09T02:07:25Z

src/index.js

    }
  }

-  const proto = CustomElement.prototype;
+  const proto = typeof CustomElementOrTagName === 'string'


Since there's already CustomElementConstructor above, could you do CustomElementConstructor.prototype?

treshugart · 2017-01-09T02:10:34Z

Hey @rnicholus sorry it took so long for me to get around to reviewing this.

rnicholus · 2017-01-09T03:29:25Z

No worries, nothing pressing here. It may be a day or so before I can get around to responding to your comments anyway, as I'm working through some other side project commitments over the next few weeks.

treshugart · 2017-01-09T22:38:05Z

PR to implement customised built-ins in the CE pfill: webcomponents/custom-elements#42.

rnicholus · 2017-01-09T22:38:52Z

PR to implement customised built-ins

Speak of the devil. Looks like that was created w/in the last hour.

treshugart · 2017-01-09T22:45:09Z

Someone else was working on converting this over to v1. Maybe we should help finish that off then polish this up. Depends on #59 which is being done as part of $58 in https://github.com/kentendo/react-integration.

rnicholus · 2017-01-09T23:08:22Z

Would love to help out. Is there a timeline for that to be completed?

off-topic: Congrats on Trello. We use JIRA + Confluence at my company. Will be interesting to see how putting all of those under one roof benefits users.

treshugart · 2017-01-09T23:13:59Z

@rnicholus re timeline, not sure if open source has one :). Availability + ASAP haha. It'd be great if you wanted to help out. Cheers!

treshugart · 2017-02-02T10:00:01Z

Hey @rnicholus I just wanted to give this a bump to see where it's at. Totes understand that you've probably been really busy. Very excited for this to go in!

rnicholus · 2017-02-02T12:27:33Z

Thanks for the reminder. It fell off my radar, but I'll add it to my task list so that does not happen again. I'll start by rereading your comments in the next day or so and take it from there

rnicholus · 2017-02-03T03:54:48Z

@treshugart I +1'd a few of your suggestions. Please let me know if there are others that you feel strongly about. Otherwise, I'll begin updating my PR with those changes post-haste.

One thing: my thoughts on custom elements have evolved a bit since I created this PR. I see the v1 spec as the future (and of course it is), and am more willing to leave behind html imports as a result. It seems possible that the HTML imports spec will be deprecated at some point, unless Firefox, Safari, and Edge choose to implement this part of the WC spec. It looks like Safari and Firefox will never implement though. So, I'm not sure if this PR will do anything more than clutter up your library.

...but maybe the is property portion of my PR is still valuable. That's a fairly trivial change though.

treshugart · 2017-02-03T04:26:13Z

@rnicholus cool, it's kinda hard for me to tell which ones weren't thumbs up'd be fore, but looks good. I can't tell from the thumbs up if you're on board with auto-applying the is attribute, re the reactify('ajax-form') stuff. What's your stance on that?

rnicholus · 2017-02-03T04:32:04Z

Re: using reactify('ajax-form'), it sounds like that would require using customElements.get() to get a handle on the constructor. If HTML imports are involved, the call may return undefined if the HTML imports hasn't completed yet. But if you aren't looking to support HTML imports, that may be a moot point.

treshugart · 2017-02-03T04:49:01Z

There's an API in the polyfill to detect when all imports have loaded and the consumer could reactify after that. That said, I don't think it's a good idea to support very contentious standards that aren't yet finalised. As long as the consumer has the control over when they call reactify() we should be future proof.

rnicholus · 2017-02-03T04:51:35Z

Makes sense. Do you think it is still important to allow users to pass tag names, instead of CE constructor functions.

treshugart · 2017-02-03T04:59:31Z

Maybe not? I don't have strong feelings about that other than it's easier to add than it is to subtract. Meaning, since the spec hasn't stabilised around HTML Imports, it might be better to wait for a bit before adding that functionality.

If this is going to block you from using it, then I think it's valuable to have that API if it's necessary. I'll let you make the judgement call here :)

rnicholus · 2017-02-03T05:21:47Z

If this is going to block you from using it

It may, without HTML import support, but I've realized that I need to embrace v1 CEs, and have already started re-writing one of my CEs to account for this.

One thing I don't know the answer to yet: What tag name will be used to render the CE by react-integration if something like reactify('ajax-form') is used? Will the tagName be 'form', or 'ajax-form'? I would think it would have to be 'form', since the tag is a <form>, not an <ajax-form>, but I'm not sure what the value of tagName would be on the constructor function to be honest. If it is instead 'form', then the user still needs to set an is prop, and we still need to be sure is is added on the initial render AFAIK.

treshugart · 2017-02-03T05:33:37Z

If we left it only as a constructor, you can use the localName and the is attribute from the constructed, temporary, element to pass that on to the virtual element without requiring a string as an argument. The only potential problem is that the element may not have been defined yet. I don't think that's a problem, though, as that responsibility should be put onto the consumer to ensure that they call reactify(Ctor) after the constructor is defined. If they know the tag name, they can also use the customElements.whenDefined(ceName) API to do this.

rnicholus · 2017-02-03T05:40:38Z

Ok, so I plan to do the following (let me know if I've missed anything, or included something redundant):

Remove all code that was put in place specifically to support HTML imports.
Auto-apply is for type extension CEs (as discussed above).
attrs property w/ an object value that holds all attributes to apply to the element.
Final docs update based on these changes.

treshugart · 2017-02-03T09:21:58Z

I do think it's important to solve your use-cases, so if we find that we do eventually require using strings, then I'm happy to discuss it at that point.

I created #65 to add an events prop just like attributes in a future PR.

One last bikeshed, what do you think about using attrs instead of attributes to match the props nomenclature? I don't have strong opinions either way, but it's worth considering at this point. I'll let you make that call.

Happy with what you've proposed in your last comment. Thank you!! :)

rnicholus · 2017-02-05T05:19:14Z

what do you think about using attrs instead of attributes to match the props nomenclature?

fine by me - just updated my checklist

This reverts commit 76390e4.

…type (CE constructor or a string/tag-name)" This reverts commit 9e2d770.

This reverts commit 82f6734.

This reverts commit 270e646.

rnicholus · 2017-02-06T04:22:11Z

At the moment, I'm blocked on the "automatically add the is property" task. A couple problems:

First, constructing a CE that extends another element results in a TypeError. For example:

class XFoo extends HTMLSpanElement {
   constructor() {
      super()
   }
}

customElements.define('x-foo', XFoo, {extends: 'span'})

// Uncaught TypeError: Illegal constructor
new XFoo()

Also, I'm not certain how to determine both the tagName ('SPAN') and the is value to apply to the element ('x-foo') given only the passed constructor. Maybe I'm missing something, but it doesn't seem like we can determine the proper value to set for is with nothing more than a constructor function.

rnicholus · 2017-02-06T05:00:49Z

After further research, even though extends was recently added to the spec, Chrome doesn't seem to support this yet. I think it's wise to hold off on this until at least the v1 polyfill has been updated. My second problem (above) remains though.

treshugart · 2017-02-08T08:39:28Z

I think it's wise to hold off on this until at least the v1 polyfill has been updated. My second problem (above) remains though.

Which part exactly? It seems that both have to do with customised built-ins. If the advice is to hold off, then wouldn't it be worth holding off on that one, too? Maybe I misunderstood.

rnicholus · 2017-02-08T12:58:21Z

I plan to hold off on the auto-"is" feature until the polyfill matches the spec. But even after the polyfill is updated, a question regarding how this can be completed remains. That question/issue, as posed earlier, is:

I'm not certain how to determine both the tagName ('SPAN') and the is value to apply to the element ('x-foo') given only the passed constructor. Maybe I'm missing something, but it doesn't seem like we can determine the proper value to set for is with nothing more than a constructor function.

treshugart · 2017-02-20T10:17:40Z

I'm not certain how to determine both the tagName ('SPAN') and the is value to apply to the element ('x-foo') given only the passed constructor. Maybe I'm missing something, but it doesn't seem like we can determine the proper value to set for is with nothing more than a constructor function.

Well, we currently construct the element to get information about it. Polyfill / native will automatically apply the is attribute, so we can use localName and getAttribute('is') (or maybe the is prop?) to get both pieces of information.

Alternatively, a better solution might be to override customElements.define() to store the information so we don't have to construct the element just to get the information.

That should work, no?

treshugart · 2017-02-20T10:18:19Z

That said, we probably don't need to do that until the polyfill supports is, correct? So can we merge this?

treshugart · 2017-03-03T00:22:44Z

Ping :). If this is gtg now, happy to merge.

rnicholus · 2017-03-03T21:15:29Z

Sorry for the delay. I don't think this is ready to merge. I was unable to complete/verify support for built-in support due to the lack of native support for built-ins in any browser and the lack of support in the polyfill. Perhaps that has changed recently or there is a workaround, but I'm unable to look into this further now. I'm hoping to get some time this month to take another look.

treshugart · 2017-03-03T21:31:24Z

@rnicholus okay, no worries. Just following up. Let me know when you need me to take another look :)

treshugart · 2018-08-05T23:48:09Z

Closing because stale. LMK if there's ever any intention to update, and thank you so much for the original PR.

rnicholus added 8 commits December 23, 2016 23:58

support for CE tag name instead of constructor

76390e4

tests won't run with anything older than node 6

9519f44

allow attributes to be attached to the CE

194096d

allow "is" prop to be attached on render for type extension CEs

42aa5ab

typescript: first param for exported function is now a union type (CE…

9e2d770

… constructor or a string/tag-name)

more unit tests

82f6734

attr- & tagName support should be part of 2.1.0

bd5e3c7

document HTML import, attribute, and type-extension support

270e646

treshugart reviewed Jan 9, 2017

View reviewed changes

treshugart mentioned this pull request Feb 3, 2017

Add events prop to add events to the CE rather than rely on heuristics for on* #65

Open

rnicholus added 6 commits February 4, 2017 23:47

Revert "support for CE tag name instead of constructor"

8d71411

This reverts commit 76390e4.

Revert "typescript: first param for exported function is now a union …

63f290e

…type (CE constructor or a string/tag-name)" This reverts commit 9e2d770.

Revert "more unit tests"

5392b27

This reverts commit 82f6734.

Revert "document HTML import, attribute, and type-extension support"

ef5f7d3

This reverts commit 270e646.

attr- & tagName support should be part of 2.1.0

d57c7db

use attrs prop to specify all attributes

a58b7ec

treshugart closed this Aug 5, 2018

Support for attributes, type-extension CEs, and those loaded by HTML import #60

Support for attributes, type-extension CEs, and those loaded by HTML import #60

Conversation

rnicholus commented Dec 26, 2016

Background

Changes in this PR

Associating attributes with custom elements

Reactifying a custom element given only its tag name

Reactifying all type-extension custom elements

treshugart commented Dec 31, 2016

rnicholus commented Dec 31, 2016 • edited Loading

treshugart commented Dec 31, 2016

rnicholus commented Dec 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnicholus Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treshugart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treshugart commented Jan 9, 2017

rnicholus commented Jan 9, 2017 • edited Loading

treshugart commented Jan 9, 2017

rnicholus commented Jan 9, 2017 • edited Loading

treshugart commented Jan 9, 2017

rnicholus commented Jan 9, 2017

treshugart commented Jan 9, 2017

treshugart commented Feb 2, 2017

rnicholus commented Feb 2, 2017

rnicholus commented Feb 3, 2017 • edited Loading

treshugart commented Feb 3, 2017

rnicholus commented Feb 3, 2017

treshugart commented Feb 3, 2017

rnicholus commented Feb 3, 2017

treshugart commented Feb 3, 2017

rnicholus commented Feb 3, 2017

treshugart commented Feb 3, 2017

rnicholus commented Feb 3, 2017 • edited Loading

treshugart commented Feb 3, 2017

rnicholus commented Feb 5, 2017

rnicholus commented Feb 6, 2017 • edited Loading

rnicholus commented Feb 6, 2017

treshugart commented Feb 8, 2017

rnicholus commented Feb 8, 2017 • edited Loading

treshugart commented Feb 20, 2017

treshugart commented Feb 20, 2017

treshugart commented Mar 3, 2017

rnicholus commented Mar 3, 2017 • edited Loading

treshugart commented Mar 3, 2017

treshugart commented Aug 5, 2018

rnicholus commented Dec 31, 2016 •

edited

Loading

rnicholus commented Dec 31, 2016 •

edited

Loading

rnicholus Jan 9, 2017 •

edited

Loading

rnicholus commented Jan 9, 2017 •

edited

Loading

rnicholus commented Jan 9, 2017 •

edited

Loading

rnicholus commented Feb 3, 2017 •

edited

Loading

rnicholus commented Feb 3, 2017 •

edited

Loading

rnicholus commented Feb 6, 2017 •

edited

Loading

rnicholus commented Feb 8, 2017 •

edited

Loading

rnicholus commented Mar 3, 2017 •

edited

Loading