Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support importing components from the react-components package #199

Closed
rolfsmeds opened this issue Dec 20, 2023 · 6 comments · Fixed by #221
Closed

Support importing components from the react-components package #199

rolfsmeds opened this issue Dec 20, 2023 · 6 comments · Fixed by #221
Assignees

Comments

@rolfsmeds
Copy link

No description provided.

@web-padawan
Copy link
Member

web-padawan commented Dec 21, 2023

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: vaadin/web-components#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
Copy link
Member

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
Copy link
Member

web-padawan commented Jan 2, 2024

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: evanw/esbuild#2983 (comment), maybe also evanw/esbuild#399.

@tomivirkki
Copy link
Member

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
Copy link
Member

web-padawan commented Jan 8, 2024

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:

  • @vaadin/charts/theme/lumo/vaadin-chart-series.js
  • @vaadin/icon/theme/lumo/vaadin-iconset.js

@web-padawan
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants