vaadin / react-components

13 stars 4 forks source link

Support importing components from the react-components package #199

Closed rolfsmeds closed 7 months ago

web-padawan commented 8 months ago

Just to clarify: the goal of this issue is to research whether we can add tree-shaking support to React components.

The idea behind it is to try moving customElements.define() to separate files and make original classes e.g. Button as side-effect free, using "sideEffects" entry in package.json of corresponding web components packages and list only files that define a custom element there (so that remaining files are considered as not containing side effects).

Prototype: https://github.com/vaadin/web-components/pull/7048

The outcome of this using esbuild looks as follows:

Bundling side-effect free component

echo "import { Button } from '@vaadin/button/theme/lumo/vaadin-button-component.js';" | npx esbuild --loader=js --bundle --outfile=out.js

  out.js  15b

Bundling component marked as side effect

echo "import { Button } from '@vaadin/button/theme/lumo/vaadin-button-component.js'; Button.register()" | npx esbuild --loader=js --bundle --outfile=out.js

  out.js  298.4kb

React integration

The next thing to prototype is how to make this work with React. In case of Button, passing it as elementClass to createComponent should be possible without calling register(), so React component could be side-effect free.

At the same time, in order to render an upgraded vaadin-button, register() would beed to be called by the user.

web-padawan commented 8 months ago

Alternative solution that we discussed is to make React wrappers use dynamic imports, so that the web component is only imported once the render function of the corresponding React component is first invoked.

Created a branch in WC repo with a quick prototype using JS. Seems to work fine: in production, only imported components are included to the bundle (in case of this example, Button and Checkbox are included but Details is not).

web-padawan commented 8 months ago

Created a WIP branch to test the dynamic import approach by passing the mock object with property names instead of the real custom element class to createComponent() function. There are some issues e.g. see the Button dev page:

The custom element definition for "vaadin-tooltip-overlay"
      was finalized before a style module was registered.
      Make sure to add component specific style modules before
      importing the corresponding custom element.

The custom element definition for "vaadin-button"
      was finalized before a style module was registered.
      Make sure to add component specific style modules before
      importing the corresponding custom element.

As far as I understand, this is related to how Vite pre-bundling dependencies work. When I remove kitchen-sink folder and then node_modules/.vite cache, the error goes away. So the issue is likely caused by the fact that vaadin-button and vaadin-tooltip are also used by other components and end up in some common JS chunks.

UPD: reproduced vaadin-button issue in a pure Vite + Lit project, using the following dynamic imports:

export class MyElement extends LitElement {
  render() {
    return html`
      <vaadin-button>Button</vaadin-button>
      <vaadin-menu-bar></vaadin-menu-bar>
    `;
  }

  async firstUpdated() {
    await Promise.all([
      import('@vaadin/menu-bar/vaadin-menu-bar.js'),
      import('@vaadin/button/vaadin-button.js'),
    ]);
  }
}

The actual issue is in esbuild. Possibly related: https://github.com/evanw/esbuild/issues/2983#issuecomment-1464960137, maybe also https://github.com/evanw/esbuild/issues/399.

tomivirkki commented 8 months ago

There are some issues e.g. see the Button dev page

The issues go away if the components are imported from "theme" (import("@vaadin/button/theme/lumo/vaadin-button.js") instead of import("@vaadin/button/vaadin-button.js").

web-padawan commented 8 months ago

The issues go away if the components are imported from "theme"

Thanks, confirmed it helps (probably because theme folders don't re-export stuff). Updated the branch.

There are some build errors with this approach UPD: solved with some tweaks to the generator script.

[dts] src/generated/Accordion.ts(5,33): error TS7016: Could not find a declaration file for module '@vaadin/accordion/theme/lumo/vaadin-accordion.js'

Also, there are some components that are missing the theme/lumo entrypoint:

web-padawan commented 8 months ago

After I bumped WC to 24.4.0-alpha2, some components in kitchen sink again have theme issue:

chunk-YO4MY45P.js?v=acf1cf7f:64 The custom element definition for "vaadin-checkbox"
      was finalized before a style module was registered.
      Make sure to add component specific style modules before
      importing the corresponding custom element.

chunk-YO4MY45P.js?v=acf1cf7f:64 The custom element definition for "vaadin-login-form-wrapper"
      was finalized before a style module was registered.
      Make sure to add component specific style modules before
      importing the corresponding custom element.

chunk-YO4MY45P.js?v=acf1cf7f:64 The custom element definition for "vaadin-chart"
      was finalized before a style module was registered.
      Make sure to add component specific style modules before
      importing the corresponding custom element.

This seems to be a bug in esbuild related to wrong import order when using dynamic imports.