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(bundling): allow proper webpack treeshaking #3248

Merged
merged 2 commits into from
Feb 23, 2022

Conversation

rwaskiewicz
Copy link
Member

@rwaskiewicz rwaskiewicz commented Feb 21, 2022

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Unit tests (npm test) were run locally and passed
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

See the GitHub Issue below. In short, applications using Stencil + Webpack are susceptible to tree shaking issues with the dist-custom-elements output target

GitHub Issue Number: 3191

What is the new behavior?

this commit solves an issue where stencil projects using webpack were
unable to treeshake properly. specifically, this is a workaround to a
webpack issue (webpack/webpack#14963) where
webpack fails to treeshake when a variable is reassigned.

with this commit, we introduce a new transformer to be run during the
typescript transpilation process, proxyCustomElement that takes a
stencil component's class initializer and hoists it as the first
argument of proxyCustomElement. this eliminates the need to reassign
the variable in the final output, which was causing code generated using
the dist-custom-elements output target to fail to treeshake when used
in a webpack project.

with the introduction of this separate transformer, the creation of the
proxyCustomElement call is removed from the
addDefineCustomElementFunctions transformer. this was done for two
reasons:

  1. separation of concerns - proxying the component is not strictly
    necessary when creating define calls
  2. proxying must occur after the initializer has been generated.
    currently, this occurs in nativeComponentTransform. therefore, this
    step must occur after nativeComponentTransform.

as a part of creating this new transformer, a new function,
createAnonymousClassMetadataProxy was created. we intentionally choose
not to use the existing proxy creation funcitons in the same file where
the new file is defiend in order to pass the class initializer directly
to our new helper function.

update-component-class has a variable statement creation call that was
modified from const to let in
6987e43. With this commit, we can
safely revert this change as we no longer redefine the variable holding
the stencil component

Does this introduce a breaking change?

  • Yes
  • No

Testing

Unit Tests

Unit tests were added, and traced with the debugger locally to verify they worked as expected.

Karma Tests

Existing karma tests surrounding defining custom elements still work

Manual Tests

autoDefineCustomElements

Manually verified autoDefineCustomElements still works as intended

Reproduction Case

Using Liam's reproduction application (see the linked GH Issue, 3191), we can reproduce the original issue like so:

git clone git@github.com:liamdebeasi/webpack-reassignment-demo.git
cd webpack-reassignment-demo
git checkout stencil
cd component-library && npm i && npm run build && npm pack
cd ../
npm i 
npm i ./component-library/component-library-0.0.1.tgz --force
npx webpack
grep -o -i -E 'Avatar Component|Badge Component' dist/main.js

The above will install the dependencies on the component library and build it, install the component library in the parent project, then run webpack. The grep command looks for components in the output of webpack. Here we see

Badge Component
Avatar Component

which is not what we want. We should only see Badge Component as its the only component used.

To test the fix, pull down this branch, npm run clean && npm ci && npm run build && npm pack to generate the tarball. Here, I'm using a fresh copy of Liam's repro:

git clone git@github.com:liamdebeasi/webpack-reassignment-demo.git
cd webpack-reassignment-demo
git checkout stencil
cd component-library
npm i
npm i PATH_TO_TARBALL --force
npm run build && npm pack
cd ../
npm i
npm i ./component-library/component-library-0.0.1.tgz --force
npx webpack
grep -o -i -E 'Avatar Component|Badge Component' dist/main.js

Again, the above will install the dependencies on the component library and build it (this time using our tarball), install the component library in the parent project, then run webpack. The only other real difference/thing worth noting is the separate npm install commands that forcefully install our special packages to make sure we have the latest & greatest. This time when we run the grep command we see "Badge Component", as intended.

Ionic Framework

Finally, as one last testing step, I installed my local tarball into Ionic Framework's core package, then npm run build && npm run test.e2e && npm run test.spec

Other Information

If it's helpful to visualize, for a simple component:

import { Component, h } from "@stencil/core";

@Component({
    tag: 'my-cmp',
})
export class MyCmp {
    render() {
        return <div>Hello</div>;
    }
}

Before this PR, the output of running the dist-custom-elements output target with stock options would be:

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

let MyCmp$1 = class extends HTMLElement {
  constructor() {
    super();
    this.__registerHost();
  }
  render() {
    return h("div", null, "Hello");
  }
};
MyCmp$1 = /*@__PURE__*/ proxyCustomElement(MyCmp$1, [0, "my-cmp"]);
function defineCustomElement$1() {
  if (typeof customElements === "undefined") {
    return;
  }
  const components = ["my-cmp"];
  components.forEach(tagName => { switch (tagName) {
    case "my-cmp":
      if (!customElements.get(tagName)) {
        customElements.define(tagName, MyCmp$1);
      }
      break;
  } });
}

const MyCmp = MyCmp$1;
const defineCustomElement = defineCustomElement$1;

export { MyCmp, defineCustomElement };

With this PR, we now generate:

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

const MyCmp$1 = /*@__PURE__*/ proxyCustomElement(class extends HTMLElement {
  constructor() {
    super();
    this.__registerHost();
  }
  render() {
    return h("div", null, "Hello");
  }
}, [0, "my-cmp"]);
function defineCustomElement$1() {
  if (typeof customElements === "undefined") {
    return;
  }
  const components = ["my-cmp"];
  components.forEach(tagName => { switch (tagName) {
    case "my-cmp":
      if (!customElements.get(tagName)) {
        customElements.define(tagName, MyCmp$1);
      }
      break;
  } });
}

const MyCmp = MyCmp$1;
const defineCustomElement = defineCustomElement$1;

export { MyCmp, defineCustomElement };

this commit solves an issue where stencil projects using webpack were
unable to treeshake properly. specifically, this is a workaround to a
webpack issue (webpack/webpack#14963) where
webpack fails to treeshake when a variable is reassigned.

with this commit, we introduce a new transformer to be run during the
typescript transpilation process, `proxyCustomElement` that takes a
stencil component's class initializer and hoists it as the first
argument of `proxyCustomElement`. this eliminates the need to reassign
the variable in the final output, which was causing code generated using
the `dist-custom-elements` output target to fail to treeshake when used
in a webpack project.

with the introduction of this separate transformer, the creation of the
`proxyCustomElement` call is removed from the
`addDefineCustomElementFunctions` transformer. this was done for two
reasons:
1. separation of concerns - proxying the component is not strictly
   necessary when creating `define` calls
2. proxying must occur after the initializer has been generated.
   currently, this occurs in `nativeComponentTransform`. therefore, this
   step must occur after `nativeComponentTransform`.

as a part of creating this new transformer, a new function,
`createAnonymousClassMetadataProxy` was created. we intentionally choose
not to use the existing proxy creation funcitons in the same file where
the new file is defiend in order to pass the class initializer directly
to our new helper function.

update-component-class has a variable statement creation call that was
modified from `const` to `let` in
6987e43. With this commit, we can
safely revert this change as we no longer redefine the variable holding
the stencil component
@rwaskiewicz rwaskiewicz marked this pull request as ready for review February 22, 2022 00:51
@rwaskiewicz rwaskiewicz requested a review from a team February 22, 2022 00:51
@rwaskiewicz rwaskiewicz merged commit 5dccc85 into main Feb 23, 2022
@rwaskiewicz rwaskiewicz deleted the rwaskiewicz/treeshaking-fix-ce branch February 23, 2022 15:53
rwaskiewicz added a commit that referenced this pull request Feb 28, 2022
…3249)

This commit adds several small refactorings that were added as a part of 
#3248. We chose a separate PR
here in that we did not feel these changes helped one understand #3248
in a meaningful way (and may have led to more churn/confusion). 

this commit adds jsdoc to parts of the compilation process that were
investigated as a part of STENCIL-325, "dist-custom-elements
re-assignment causes treeshaking to fail with webpack"

remove an unused argument from the `generateEntryPoint` function for the
`dist-custom-elements` output target

remove an unused argument to the helper function for generating virtual
modules for the `dist-custom-elements` output target

remove the `exp` local variable from the helper function for generating
entrypoints for the `dist-custom-elements` output target. this refactor
is believed to be safe as:
- the array is created locally within the helper function
- nothing is ever added/removed from the array
- the return value of the function is the result of spreading the string
  values in the array into a new array. since this array is always
  empty, this becomes a no-op

update existing generation of `proxyCustomElement` to use newer
`ts.factory` based calls. the replaced calls are drop in replacements
for their predecessors

rename the function for retrieving custom transformers for the
`dist-custom-elements` output target, as it was previously referring to
the `dist-custom-elements-bundle` output target
alicewriteswrongs added a commit that referenced this pull request Mar 22, 2023
This fixes an issue (documented in #3434) where when using
`@testing-libary/dom` to test a Stencil component wrapped with the React
framework wrappers could produce an infinite loop that would cause the
tests to fail.

The issue relates to an assumption that `@testing-library/dom` makes
about the `.name` property on the constructor for a custom element. In
particular, `@testing-library/dom` expects the property to be defined
here:

https://github.com/testing-library/dom-testing-library/blob/fb069c93983bc0300a6e1c91bdec5bf9443b5286/src/DOMElementFilter.ts#L198

When building with the `dist-custom-elements` output target we create an
anonymous class expression and inline it into a call in the emitted JS
to `proxyCustomElement`, like this:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

We made a change (#3248) to fix an issue (#3191) with webpack
treeshaking where if we didn't inline an anonymous class expression like
this we would get improper tree shaking in webpack.

One consequence, however, of an _anonymous_ inline class expression is
that the `.name` property on its constructor is going to be `""`, which
fails the false-ey test in `@testing-library/dom` referenced above.

So in order to fix the issue we can simply insert a name so that the
inlined class expression is no longer anonymous, like so:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class MyComponent extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

This fixes the issue with infinite loops while testing with the React
wrapper. Additionally, using the reproduction case provided for #3191 we
can confirm that this does not cause a regression with respect the
previous fix for the webpack treeshaking issue.
alicewriteswrongs added a commit that referenced this pull request Mar 22, 2023
This fixes an issue (documented in #3434) where when using
`@testing-libary/dom` to test a Stencil component wrapped with the React
framework wrappers could produce an infinite loop that would cause the
tests to fail.

The issue relates to an assumption that `@testing-library/dom` makes
about the `.name` property on the constructor for a custom element. In
particular, `@testing-library/dom` expects the property to be truthy
here:

https://github.com/testing-library/dom-testing-library/blob/fb069c93983bc0300a6e1c91bdec5bf9443b5286/src/DOMElementFilter.ts#L198

When building with the `dist-custom-elements` output target we create an
anonymous class expression and inline it into a call in the emitted JS
to `proxyCustomElement`, like this:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

We made a change (#3248) to fix an issue (#3191) with webpack
treeshaking where if we didn't inline an anonymous class expression like
this we would get improper tree shaking in webpack.

One consequence, however, of an _anonymous_ inline class expression is
that the `.name` property on its constructor is going to be `""`, which
fails the false-ey test in `@testing-library/dom` referenced above.

So in order to fix the issue we can simply insert a name so that the
inlined class expression is no longer anonymous, like so:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class MyComponent extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

This fixes the issue with infinite loops while testing with the React
wrapper. Additionally, using the reproduction case provided for #3191 we
can confirm that this does not cause a regression with respect the
previous fix for the webpack treeshaking issue.
alicewriteswrongs added a commit that referenced this pull request Mar 23, 2023
This fixes an issue (documented in #3434) where when using
`@testing-libary/dom` to test a Stencil component wrapped with the React
framework wrappers could produce an infinite loop that would cause the
tests to fail.

The issue relates to an assumption that `@testing-library/dom` makes
about the `.name` property on the constructor for a custom element. In
particular, `@testing-library/dom` expects the property to be truthy
here:

https://github.com/testing-library/dom-testing-library/blob/fb069c93983bc0300a6e1c91bdec5bf9443b5286/src/DOMElementFilter.ts#L198

When building with the `dist-custom-elements` output target we create an
anonymous class expression and inline it into a call in the emitted JS
to `proxyCustomElement`, like this:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

We made a change (#3248) to fix an issue (#3191) with webpack
treeshaking where if we didn't inline an anonymous class expression like
this we would get improper tree shaking in webpack.

One consequence, however, of an _anonymous_ inline class expression is
that the `.name` property on its constructor is going to be `""`, which
fails the false-ey test in `@testing-library/dom` referenced above.

So in order to fix the issue we can simply insert a name so that the
inlined class expression is no longer anonymous, like so:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class MyComponent extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

This fixes the issue with infinite loops while testing with the React
wrapper. Additionally, using the reproduction case provided for #3191 we
can confirm that this does not cause a regression with respect the
previous fix for the webpack treeshaking issue.
alicewriteswrongs added a commit that referenced this pull request Mar 24, 2023
This fixes an issue (documented in #3434) where when using
`@testing-libary/dom` to test a Stencil component wrapped with the React
framework wrappers could produce an infinite loop that would cause the
tests to fail.

The issue relates to an assumption that `@testing-library/dom` makes
about the `.name` property on the constructor for a custom element. In
particular, `@testing-library/dom` expects the property to be truthy
here:

https://github.com/testing-library/dom-testing-library/blob/fb069c93983bc0300a6e1c91bdec5bf9443b5286/src/DOMElementFilter.ts#L198

When building with the `dist-custom-elements` output target we create an
anonymous class expression and inline it into a call in the emitted JS
to `proxyCustomElement`, like this:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

We made a change (#3248) to fix an issue (#3191) with webpack
treeshaking where if we didn't inline an anonymous class expression like
this we would get improper tree shaking in webpack.

One consequence, however, of an _anonymous_ inline class expression is
that the `.name` property on its constructor is going to be `""`, which
fails the false-ey test in `@testing-library/dom` referenced above.

So in order to fix the issue we can simply insert a name so that the
inlined class expression is no longer anonymous, like so:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class MyComponent extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

This fixes the issue with infinite loops while testing with the React
wrapper. Additionally, using the reproduction case provided for #3191 we
can confirm that this does not cause a regression with respect the
previous fix for the webpack treeshaking issue.
alicewriteswrongs added a commit that referenced this pull request Mar 24, 2023
This fixes an issue (documented in #3434) where when using
`@testing-libary/dom` to test a Stencil component wrapped with the React
framework wrappers could produce an infinite loop that would cause the
tests to fail.

The issue relates to an assumption that `@testing-library/dom` makes
about the `.name` property on the constructor for a custom element. In
particular, `@testing-library/dom` expects the property to be truthy
here:

https://github.com/testing-library/dom-testing-library/blob/fb069c93983bc0300a6e1c91bdec5bf9443b5286/src/DOMElementFilter.ts#L198

When building with the `dist-custom-elements` output target we create an
anonymous class expression and inline it into a call in the emitted JS
to `proxyCustomElement`, like this:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

We made a change (#3248) to fix an issue (#3191) with webpack
treeshaking where if we didn't inline an anonymous class expression like
this we would get improper tree shaking in webpack.

One consequence, however, of an _anonymous_ inline class expression is
that the `.name` property on its constructor is going to be `""`, which
fails the false-ey test in `@testing-library/dom` referenced above.

So in order to fix the issue we can simply insert a name so that the
inlined class expression is no longer anonymous, like so:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class MyComponent extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

This fixes the issue with infinite loops while testing with the React
wrapper. Additionally, using the reproduction case provided for #3191 we
can confirm that this does not cause a regression with respect the
previous fix for the webpack treeshaking issue.
alicewriteswrongs added a commit that referenced this pull request May 30, 2023
This fixes an issue (documented in #3434) where when using
`@testing-libary/dom` to test a Stencil component wrapped with the React
framework wrappers could produce an infinite loop that would cause the
tests to fail.

The issue relates to an assumption that `@testing-library/dom` makes
about the `.name` property on the constructor for a custom element. In
particular, `@testing-library/dom` expects the property to be truthy
here:

https://github.com/testing-library/dom-testing-library/blob/fb069c93983bc0300a6e1c91bdec5bf9443b5286/src/DOMElementFilter.ts#L198

When building with the `dist-custom-elements` output target we create an
anonymous class expression and inline it into a call in the emitted JS
to `proxyCustomElement`, like this:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

We made a change (#3248) to fix an issue (#3191) with webpack
treeshaking where if we didn't inline an anonymous class expression like
this we would get improper tree shaking in webpack.

One consequence, however, of an _anonymous_ inline class expression is
that the `.name` property on its constructor is going to be `""`, which
fails the false-ey test in `@testing-library/dom` referenced above.

So in order to fix the issue we can simply insert a name so that the
inlined class expression is no longer anonymous, like so:

```js
const MyComponent$1 = /*@__PURE__*/ proxyCustomElement(
  class MyComponent extends HTMLElement {
    ...
  },
  [1, "my-component", {}]
);
```

This fixes the issue with infinite loops while testing with the React
wrapper. Additionally, using the reproduction case provided for #3191 we
can confirm that this does not cause a regression with respect the
previous fix for the webpack treeshaking issue.
github-merge-queue bot pushed a commit to ionic-team/ionic-framework that referenced this pull request Oct 18, 2023
Issue number: resolves #28358

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->


28f2ec9
exposed a (possible) `ng-packagr` bug where the form control components
were being re-assigned, which breaks treeshaking. These components were
considered side effects and were always being pulled into the bundle.

This resulted in a higher than expected bundle size. This issue appears
to be caused by using 2 decorators **and** referring to the class in
`useExisting` (for providers). Doing just one of these does not
reproduce the issue.

The compiled output looks something like this:

```typescript
let IonToggle = IonToggle_1 = /*@__PURE__*/ class IonToggle extends ValueAccessor {
    constructor(c, r, z, injector) {
        super(injector, r);
        this.z = z;
        c.detach();
        this.el = r.nativeElement;
        proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']);
    }
    writeValue(value) {
        this.elementRef.nativeElement.checked = this.lastValue = value;
        setIonicClasses(this.elementRef);
    }
    handleIonChange(el) {
        this.handleValueChange(el, el.checked);
    }
};
/** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component });
/** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [
        {
            provide: NG_VALUE_ACCESSOR,
            useExisting: IonToggle_1,
            multi: true,
        },
    ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush });
IonToggle = IonToggle_1 = __decorate([
    ProxyCmp({
        defineCustomElementFn: defineCustomElement$1i,
        inputs: TOGGLE_INPUTS,
    })
], IonToggle);
```

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Removed the `ProxyCmp` usage in favor of manually calling proxyInputs
and proxyMethods.
-  Also saw that select was missing a form control test, so I added one

The compiled code now looks something like this:

```typescript
class IonToggle extends ValueAccessor {
    constructor(c, r, z, injector) {
        super(injector, r);
        this.z = z;
        defineCustomElement$1i();
        proxyInputs(IonToggle, TOGGLE_INPUTS);
        c.detach();
        this.el = r.nativeElement;
        proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']);
    }
    writeValue(value) {
        this.elementRef.nativeElement.checked = this.lastValue = value;
        setIonicClasses(this.elementRef);
    }
    handleIonChange(el) {
        this.handleValueChange(el, el.checked);
    }
}
/** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component });
/** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [
        {
            provide: NG_VALUE_ACCESSOR,
            useExisting: IonToggle,
            multi: true,
        },
    ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush });
```

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Ryan provided some context on a related Stencil bug where doing
reassignments broke treeshaking in Webpack. While the source of this bug
is not Stencil, understanding the Stencil bug helped me better
understand this issue:

ionic-team/stencil#3191
ionic-team/stencil#3248
ionic-team/stencil#4188 (fixes an issue
introduced in the above stencil PR)

Dev build: `7.5.1-dev.11697480817.10fa2601`
sean-perkins pushed a commit to ionic-team/ionic-framework that referenced this pull request Oct 27, 2023
Issue number: resolves #28358

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->


28f2ec9
exposed a (possible) `ng-packagr` bug where the form control components
were being re-assigned, which breaks treeshaking. These components were
considered side effects and were always being pulled into the bundle.

This resulted in a higher than expected bundle size. This issue appears
to be caused by using 2 decorators **and** referring to the class in
`useExisting` (for providers). Doing just one of these does not
reproduce the issue.

The compiled output looks something like this:

```typescript
let IonToggle = IonToggle_1 = /*@__PURE__*/ class IonToggle extends ValueAccessor {
    constructor(c, r, z, injector) {
        super(injector, r);
        this.z = z;
        c.detach();
        this.el = r.nativeElement;
        proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']);
    }
    writeValue(value) {
        this.elementRef.nativeElement.checked = this.lastValue = value;
        setIonicClasses(this.elementRef);
    }
    handleIonChange(el) {
        this.handleValueChange(el, el.checked);
    }
};
/** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component });
/** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [
        {
            provide: NG_VALUE_ACCESSOR,
            useExisting: IonToggle_1,
            multi: true,
        },
    ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush });
IonToggle = IonToggle_1 = __decorate([
    ProxyCmp({
        defineCustomElementFn: defineCustomElement$1i,
        inputs: TOGGLE_INPUTS,
    })
], IonToggle);
```

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Removed the `ProxyCmp` usage in favor of manually calling proxyInputs
and proxyMethods.
-  Also saw that select was missing a form control test, so I added one

The compiled code now looks something like this:

```typescript
class IonToggle extends ValueAccessor {
    constructor(c, r, z, injector) {
        super(injector, r);
        this.z = z;
        defineCustomElement$1i();
        proxyInputs(IonToggle, TOGGLE_INPUTS);
        c.detach();
        this.el = r.nativeElement;
        proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']);
    }
    writeValue(value) {
        this.elementRef.nativeElement.checked = this.lastValue = value;
        setIonicClasses(this.elementRef);
    }
    handleIonChange(el) {
        this.handleValueChange(el, el.checked);
    }
}
/** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component });
/** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [
        {
            provide: NG_VALUE_ACCESSOR,
            useExisting: IonToggle,
            multi: true,
        },
    ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush });
```

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Ryan provided some context on a related Stencil bug where doing
reassignments broke treeshaking in Webpack. While the source of this bug
is not Stencil, understanding the Stencil bug helped me better
understand this issue:

ionic-team/stencil#3191
ionic-team/stencil#3248
ionic-team/stencil#4188 (fixes an issue
introduced in the above stencil PR)

Dev build: `7.5.1-dev.11697480817.10fa2601`
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.

2 participants