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

fix(angular, react, vue): use defineCustomElement import to improve treeshaking #208

Merged
merged 18 commits into from
Dec 6, 2021

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Dec 1, 2021

resolves #199

When using autoDefineCustomElements, Stencil automatically generates a defineCustomElement() call in the body of a component file. This is a side effect, which causes bundlers to pull in the entire file when building. When importing components from a single entry point (I.e. import { IonButton } from '@ionic/vue';), this causes bundlers to bundle every component with your application, regardless of whether or not the user is actually using the component in their application.

For example, the entry point of @ionic/vue looks something like this:

index.esm.js

import { IonAvatar as IonAvatar$1 } from '@ionic/core/component/ion-avatar.js';
import { IonBadge as IonBadge$1 } from '@ionic/core/component/ion-badge.js';

const defineComponent = (tagName, customElement) => {
  // Define Custom Element
  // Define and return the Vue wrapper
}

const IonAvatar = defineComponent('ion-avatar', IonAvatar$1);
const IonBadge = defineComponent('ion-badge', IonBadge$1);

export { IonAvatar, IonBadge }

and the file for each component looks something like this (IonAvatar used as example):

import { HTMLElement, proxyCustomElement } from '@stencil/core/internal/client';

let Avatar = class extends HTMLElement {
  ...
};
Avatar = /*@__PURE__*/ proxyCustomElement(Avatar, [33, "ion-avatar"]);
function defineCustomElement$1() {
  if (typeof customElements === "undefined") {
    return;
  }
  const components = ["ion-avatar"];
  components.forEach(tagName => { switch (tagName) {
    case "ion-avatar":
      if (!customElements.get(tagName)) {
        customElements.define(tagName, Avatar);
      }
      break;
  } });
}

const IonAvatar = Avatar;
const defineCustomElement = defineCustomElement$1;

defineCustomElement$1();

export { IonAvatar, defineCustomElement };

When bundlers look at index.esm.js, they need to evaluate both ion-avatar.js and ion-badge.js. Calling defineCustomElement$1(); in each of the component files creates a side effect, resulting in both ion-avatar and ion-badge being added to an application bundle, even if ion-avatar is the only component being used.


The wrappers have a secondary defect where if you turn off autoDefineCustomElements, child components are never defined because the wrappers only define the parent component.

The wrappers found in this repo do the following to define the components:

if (
    customElement !== undefined &&
    typeof customElements !== 'undefined' &&
    !customElements.get(tagName)
  ) {
    customElements.define(tagName, customElement);
  }

This PR seeks to solve both the wrapper defect and the treeshaking issue by having the wrappers import defineCustomElement for each component and call it when the parent component is used.

The one thing to note is that this PR would require the minimum Stencil version to be 2.10.0 as I believe that is the version where the individual defineCustomElement functions were generated.


Note: This PR does not resolve parts of #198 which describes a potential side effect created by proxyCustomElement. My initial investigation found that this only impacts Webpack, so Rollup/ESBuild/others should not be impacted by #198.

const decorator = function (cls: any) {
const { tagName, customElement, inputs, methods } = opts;

defineCustomElement(tagName, customElement);
if (defineCustomElementFn !== undefined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another const function called defineCustomElements defined above, so that's why I called this defineCustomElementFn.

@liamdebeasi liamdebeasi requested a review from a team December 1, 2021 14:50
Copy link
Contributor

@willmartian willmartian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and makes sense. Thanks for the thorough write-up. 🚀

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 this pull request may close these issues.

React wrapper treeshaking (dist custom elements) is broken in the Stencil latest release 2.9.0
2 participants