-
Notifications
You must be signed in to change notification settings - Fork 789
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
bug: dist-custom-elements re-assignment causes treeshaking to fail with webpack #3191
Comments
Thanks for the issue! This issue has been labeled as |
Hi, would like to know if this issues is prioritized in an upcoming release. |
This bug has been resolved in Stencil v2.14.1, which was released today (2022.03.07). I'm going to close this issue and mark it as shipped, thanks again! |
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.
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.
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.
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.
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.
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.
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`
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`
Prerequisites
Stencil Version
2.12.0
Current Behavior
When using a
dist-custom-elements
Stencil library consumed through a single entry point, Webpack does not treeshake components properly causing all components to be bundled with an application.This does not happen when consuming components by importing them directly from their built files.
Expected Behavior
I expect Webpack to only include the components I am using in my application when building.
Steps to Reproduce
stencil
branch).cd component-library && npm install && npm run build
to build the Stencil component library.component-library
directory, runnpm pack
.npm install
.src/index.js
. Observe that onlyAppBadge
is imported.npx webpack
.grep -o -i -E 'Avatar Component|Badge Component' dist/main.js
. This will search the built output for the"Avatar Component"
and"Badge Component"
strings. These strings are found in theapp-avatar.js
andapp-badge.js
files and are used to quickly tell if Webpack is bundling the components when building. Observe that both strings appear, meaning bothAppBadge
andAppAvatar
are bundled with the app.node_modules/component-library/components/app-badge.js
with the following modifications:node_modules/component-library/components/app-avatar.js
with the following modifications:npx webpack
.grep -o -i -E 'Avatar Component|Badge Component' dist/main.js
. Observe that only the"Badge Component"
string appears, meaning onlyAppBadge
is bundled.Code Reproduction URL
https://github.com/liamdebeasi/webpack-reassignment-demo/tree/stencil
Additional Information
The re-assignment is something Webpack does not account for when building according to webpack/webpack#14962 (comment). I have filed an issue on the Webpack repo here: webpack/webpack#14963.
This issue does not impact non-Webpack bundlers such as Rollup.
This is currently impacting Ionic 6 applications where all Ionic components are being added to an app when consuming them through the
@ionic/react
entry point.The text was updated successfully, but these errors were encountered: