webcomponents / react-integration

Converts web components into React components so that you can use them as first class citizens in your React components.
MIT License
306 stars 31 forks source link

Allows to pass to reactify the tagName instead of the CustomElement #38

Closed andreawyss closed 8 years ago

andreawyss commented 8 years ago

Allows to pass to reactify the tagName instead of the CustomElement class. Avoids creating an instance of the component just to get the tagName. Makes it possible to use this with Web.Components written in TypeScript.

stevemao commented 8 years ago

Can you add a test for this?

andreawyss commented 8 years ago

This only work with custom element V1. At the same time this is to work around the typescript issue 7574 that is being work on: https://github.com/Microsoft/TypeScript/issues/7574

I don't know how to add custom element V1 this to the test. This is what I have so far:

describe('reactify-with-tag-name', () => { it('should be able to reactify with tagName', () => {

window.customElements.define('x-comp', {
  prototype: Object.create(HTMLElement.prototype)
});

const Comp = reactify("x-comp", {React, ReactDOM});
ReactDOM.render(React.createElement(Comp), window.fixture);
expect(Comp.displayName).to.equal('XComp');

}); });

stevemao commented 8 years ago

I think your test looks good. Also it doesn't look like it's just for typescript? It's pretty generic (CustomElement could be either a string or class).

andreawyss commented 8 years ago

I agree it is very generic but, customElement V1 or a customElement V1 polyfill must be in place. My PR code uses customElements.get() and the test uses customElements.define(). The problem is that I don't know how to add the polyFill to the test. The test fails without a polyfill like this one: https://github.com/webcomponents/custom-elements

treshugart commented 8 years ago

@andreawyss you can import https://github.com/skatejs/web-components to the package.json and then import it in the main test entry point (test/unit.js).

I've thought about doing this in the past but decided against it for a couple reasons:

  1. You can create millions of elements a second (you may have hundreds of components on the page at once and it's likely it wouldn't cause any measurable performance issues) and your components shouldn't have side effects, or at least any side effects that don't happen until connectedCallback is fired. You also shouldn't be doing any heavy lifting until the component is actually in the DOM. Unfortunately these are best-practices that have largely been left out of the greater web component ecosystem docs and resources.
  2. I'm unsure if the API will ever internally change to require more information from the constructor such as observedAttributes et al.

There's currently no spec on how to get the localName of a component using only the constructor and I'm somewhat hesitant to add this because of point 2.

I've raised an issue and a modest proposal in https://github.com/w3c/webcomponents/issues/566 for doing this without having to construct the element.

Any counter arguments? Thoughts? This really feels like something TypeScript needs to fix, however, including the polyfill might fix it.

andreawyss commented 8 years ago

I agree that this is just a workaround for a TypeScript bug. This should be fixed soon in next version of TypeScript since this pull request is addressing this issue: https://github.com/Microsoft/TypeScript/pull/10762

Therefore I will Close this PR. Thank you.

treshugart commented 8 years ago

Awesome! Thanks, @andreawyss!