BuilderIO / mitosis

Write components once, run everywhere. Compiles to React, Vue, Qwik, Solid, Angular, Svelte, and more.
https://mitosis.builder.io
MIT License
11.84k stars 513 forks source link

webcomponents: unnecessary wrappers #284

Open WebReflection opened 2 years ago

WebReflection commented 2 years ago

Describe the bug The outcome produces a builder-component which wouldn't exist in the React counterpart, meaning that each component will result in DOM bloat for no reason, nor benefits.

To Reproduce Steps to reproduce the behavior:

  1. visit this demo example
  2. inspect the DOM
  3. note builder-component wraps the DIV, which is the only thing React would render instead

Expected behavior A div builtin extend that is="builder-component-xxx" where xxx is the unique relation between the MyComponent function and the outcome in the DOM.

Additional context The only way to represent React output as Custom Element is to use builtin extends, available in every browser but Safari, which can easily be polyfilled if necessary in case window.safari or window.webkit exists, through import maps or ad-hoc builds for these targets.

The output should be a custom elements which represents what the React function would return and update its state/view whenever a state is triggered.

This is because it's impossible otherwise to represent special nodes such as td, li, tr, option, or others, if these get wrapped by unrelated nodes.

Changing div with td as returned node will indeed fail.

steve8708 commented 2 years ago

When you say “inspect the dom”, are you referring to the builder preview? The preview uses a wrapper just for the visual editor to work. It should not be in the mitosis generated code unless I am mistaken and you can point us to where the mitosis output code includes this

WebReflection commented 2 years ago

@steve8708 when I say inspect the DOM I mean with devtools DOM inspector, but it's trivial to understand the output will have a <builder-component> element and that cannot possibly work in cases like this one.

You cannot translate React into Custom Elements wrappers, because not all elements can be wrapped, and wrapping also creates unnecessary DOM bloat.

steve8708 commented 2 years ago

Hey @WebReflection I still think there is some confusion here. When editing visually with Builder.io, we rap the editable region in a . But Mitosis will never output code to have this (that I know of). I am guessing you are inspecting the dom in that preview iframe but that adds wrapping for the interactive editor to work that doesn't show up in the output code. In the example you linked to, this is the output code which has no

![Uploading image.png…]()

Let me know if I am mistaken here though

steve8708 commented 2 years ago

Oops I am on clunky plane wifi and think my image never uploaded, I'll paste here as text instead. This is what mitosis actually generates:

/**
 * Usage:
 *
 *  <my-component></my-component>
 *
 */
class MyComponent extends HTMLElement {
  constructor() {
    super();

    const self = this;
    this.state = { name: "Steve" };

    // Event handler for 'input' event on input-1
    this.onInput1Input = (event) => {
      this.state.name = event.target.value;
      this.update();
    };
  }

  connectedCallback() {
    this.innerHTML = `
      <div>
        <input class="input" data-name="input-1" />

        Hello! I can run in React, Vue, Solid, or Liquid!
      </div>
      <style>
        .input {
          color: red;
        }
      </style>`;
    this.update();
  }

  update() {
    this.querySelectorAll("[data-name='input-1']").forEach((el) => {
      el.value = this.state.name;
      el.removeEventListener("input", this.onInput1Input);
      el.addEventListener("input", this.onInput1Input);
    });
  }
}

customElements.define("my-component", MyComponent);

The is just an alternative means of rendering for visual editing that has some slight differences from the mitosis output, and shouldn't affect mitoisis's generated code

WebReflection commented 2 years ago

this is what I mean: the outcome creates a wrapper of what React would've created ...

nope

nope-nope

This does not work when components cannot be wrapped, as it is for <table>, <td>, <li>, <option> and other standrd HTML element otherwise possible with React.

Indeed, this example/demo is not possible with current mitosis webcomponents target, but if it is, please show me that exact demo outcome after mitosis, where the rendered output should be exactly this one, as it is for its React counterpart:

<body>
  <table>
    <tr>
      <th>Name</th>
      <th>Age</th>
    </tr>
    <tr>
      <td>Custom Element</td>
      <td>8</td>
    </tr>
  </table>
</body>

I hope this is now clear.

steve8708 commented 2 years ago

This absolutely clarifies, thanks so much for taking the time to elaborate @WebReflection. I am not married to the current approach at all, would be absolutely be game for your propose change. Please feel free to improve this and shoot over a PR!

Also, you should join our discord where we have a #mitosis->general channel for chatting on this stuff as well, would love to collaborate closely and love your ideas, you can join here

WebReflection commented 2 years ago

I'm afraid I don't have much time these days to join channels, plus I've solved this long time ago through my libraries!

However, I'd be more than happy to write down what this project should consider, in order to properly translate React to WC, as long as this project also welcomes custom elements builtin extends, and optional Safari polyfill, otherwise we'll both waste time around this topic 'cause nothing else would practically work 😉

samijaber commented 2 years ago

I'm afraid I don't have much time these days

@WebReflection Congratulations! 😄

However, I'd be more than happy to write down what this project should consider, in order to properly translate React to WC

That would be greatly appreciated! Knowledge from domain experts such as yourself can be tremendously helpful for us to implement the Mitosis WC generator. 🙏🏽

WebReflection commented 2 years ago

@samijaber thanks! About the example, this is just a quick summary of what should be done, but it requires special care for props and it doesn't include fragments possibility. Bear in mind 1:1 React to WC is extremely difficult to have, simply because the paradigm is different, and so are the primitives used behind the scene.

React

import { useState } from "@builder.io/mitosis";

export default function MyComponent(props) {
  const [name, setName] = useState("Steve");

  return (
    <div value={props.name} onClick={(event) => console.log(event.type)}>
      <input
        css={{
          color: "red",
        }}
        value={name}
        onChange={(event) => setName(event.target.value)}
      />
      Hello! I can run in React, Vue, Solid, or Liquid!
    </div>
  );
}

WC

/**
 * Usage:
 *
 *  <div is="my-component"></div>
 *
 */
 class MyComponent extends HTMLDivElement {
  constructor() {
    super();

    // needed only if global style is needed/used (see connectedCallback)
    this.dataset.name = 'div-1';

    this.state = { name: "Steve" };

    // Event handler for 'click' event on div-1
    this.onDiv1Click = (event) => {
      console.log(event.type);
    };

    // Event handler for 'input' event on input-1
    this.onInput1Input = (event) => {
      this.state.name = event.target.value;
      this.update();
    };
  }

  // currently, the injected style would affect all inputs out there
  // so, there are two different possible CSS solutions for the input:
  //  * inline style (faster, simpler, easier to implement)
  //  * global style with a specifier:
  //      <style>
  //        div[is="my-component"][data-name="div-1"] input {
  //          color: red;
  //        }
  //      </style>
  connectedCallback() {
    this.innerHTML = `
        <input style="color: red;" data-name="input-1" />

        Hello! I can run in React, Vue, Solid, or Liquid!
    `;
    this.update();
  }

  update() {
    // the values related to the element itself should be addressed directly
    this.value = this.props.name;
    this.removeEventListener("click", this.onDiv1Click);
    this.addEventListener("click", this.onDiv1Click);

    // values related to inner nodes should, instead, be added at runtime
    this.querySelectorAll("[data-name='input-1']").forEach((el) => {
      el.value = this.state.name;
      el.removeEventListener("input", this.onInput1Input);
      el.addEventListener("input", this.onInput1Input);
    });
  }
}

customElements.define("my-component", MyComponent, {extends: 'div'});
// the render should now create a <div is="my-component"> instead
// and it should attach right after `props` to it

Now, with this kind of outcome, we can represent pretty much every JSX except for fragments, which cannot really be represented as single WC because we can't extend, or define, a fragment in a declarative way, so that use case might require a special <partial-content> Custom Element that takes its previous node and append to it its content.

Last, but not least, it should be possible to have inner components through "virtual slots" injected once with the innerHTML on connected callback, and updated later on each time it's needed (propagate updates down those references).

So, updates propagates down but there's no need to propagate up, except for events that bubble already so ... it's all good.

I hope this give you a better idea of what I mean when I say that currently mitosis does not provide React to WC conversion.