preactjs / preact-custom-element

Wrap your component up as a custom element
MIT License
352 stars 51 forks source link

Allowing any attribute to passed to the Custom Element? #49

Open wbern opened 3 years ago

wbern commented 3 years ago

Hi! Thanks for all the hard work!

My org is unable to pass Props from the React component to the register() function for reasons (mainly regarding Functional Components and TypeScript, and an ambition to not have to modify the underlying component because we have quite many).

I was wondering, would it be possible to let us call register(), and let the default behavior of undefined to the attributes parameter, mean that all attributes set on the Custom Element will be passed down?

My team thought this may be an issue for property assignment somehow, but I'd like to bring this up for discussion since we're at a cross-roads at the moment due to this on what to do (we've even gone as far as made TypeScript AST parsing for our typescript-related props type declarations)

marvinhagemeister commented 3 years ago

@wbern Can you share a snippet that shows how you're calling register() and what the component class/function looks like?

wbern commented 3 years ago

@marvinhagemeister Here is the component in its entirety, plus the register() call. :-)

import React, { useState } from 'react';
import register from 'preact-custom-element/src/index.js';
import AccordionStateless from './AccordionStateless/AccordionStateless';
import { CreateProps } from '../../types/utils/CreateProps';

type AccordionProps = CreateProps<
  {
    /** Visible heading in the Accordion */
    heading: string;
    /** Controls i3f the Accordion is initially expanded or not */
    initiallyExpanded?: boolean;
    /** Define if Heading font style is Pebble */
    isHeadingPebble?: boolean;
  },
  typeof AccordionStateless,
  'onClick' | 'open'
>;

const Accordion: React.FunctionComponent<AccordionProps> = ({
  children,
  initiallyExpanded,
  ...props
}) => {
  const [open, setOpen] = useState(initiallyExpanded || false);

  const toggleExpanded = () => {
    setOpen(!open);
  };

  return (
    <AccordionStateless onClick={toggleExpanded} open={open} {...props}>
      {children}
    </AccordionStateless>
  );
};

export default Accordion;

window.HMRReactComponentRefs = window.HMRReactComponentRefs || {};
window.HMRReactComponentRefs.Accordion = Accordion;

if (customElements.get('tc-accordion') === undefined) {
  register(
    window.HMRReactComponentRefs.Accordion,
    'tc-accordion',
    // this part is a pain to put in for all our components manually
    ['heading', 'initiallyExpanded', 'isHeadingPebble', 'onClick'],
    { shadow: true, injectGlobalStyles: true }
  );
}
raihle commented 3 years ago

@marvinhagemeister I'm working with @wbern on the same project. The issue is that the components we want to wrap are using TypeScript typings rather than React's PropTypes, and we would like to avoid duplication. https://github.com/milesj/babel-plugin-typescript-to-proptypes is not able to pull out the type information (it only seems to work with very simple types). I'm working on parsing the TypeScript AST to get the prop names, but it is slow going.

The list of props above is heavily abbreviated (let's call it "optimized"), since we should in principle be forwarding every legal DOM property...

wbern commented 3 years ago

Here is an example of what we've managed to pull out from an AST parser, including all legal DOM attributes.

It feels a little wrong to pursue this approach just because we don't want to whitelist manually.

> node dist/parser.js ds
{
  file: '../ds/src/index.js',
  components: [
    {
      component: 'Accordion',
      from: "'./components/Accordion/Accordion'",
      type: 'function',
      props: [
        'heading',
        'initiallyExpanded',
        'isHeadingPebble',
        'slot',
        'style',
        'title',
        'color',
        'className',
        'hidden',
        'id',
        ... 267 more items
      ]
    },
    {
      component: 'AccordionStateless',
      from: "'./components/Accordion/AccordionStateless/AccordionStateless'",
      type: 'function',
      props: [
        'heading',
        'initiallyExpanded',
        'isHeadingPebble',
        'open',
        'onClick',
        'slot',
        'style',
        'title',
        'color',
        'className',
        ... 269 more items
      ]
    },
    ...
marvinhagemeister commented 3 years ago

@raihle Agree, the observedAttributes thing about web-components made me scratch my head too. The only explainer for that I can find is this comment. It seems to imply that the main reason was to avoid expensive style recalculations when the style property changes.

Doing a bit more reading on the topic, I'm wondering if we default to listening to all attributes via a MutationObserver instance, similar to the solution described here: https://github.com/w3c/webcomponents/issues/565#issuecomment-345556883 .

I don't expect devs to use style in association with Preact components wrapped as web-components. Even in code bases on GitHub that make extensive use of web-components I don't see that. So I'm really leaning to observe all attributes by default.

wbern commented 3 years ago

Could be worth looking into similar wrappers, perhaps? Like vue's wc wrapper.

wbern commented 3 years ago

This is a good example.

https://github.com/vuejs/vue-web-component-wrapper/blob/e2b84569c4671a7ea451b3887840533261e71715/src/index.js#L105