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

bug: side effects with reassignment in ionic 6 + webpack, vite #24280

Closed
4 of 6 tasks
ptmkenny opened this issue Nov 27, 2021 · 19 comments · Fixed by #24895
Closed
4 of 6 tasks

bug: side effects with reassignment in ionic 6 + webpack, vite #24280

ptmkenny opened this issue Nov 27, 2021 · 19 comments · Fixed by #24895
Labels
package: react @ionic/react package type: bug a confirmed bug report v6 issues specific to Framework v6

Comments

@ptmkenny
Copy link

ptmkenny commented Nov 27, 2021

Prerequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x

Current Behavior

I have an Ionic React v5 app that I upgraded to v6. When I did so and inspected the bundle size:

react-scripts build --stats; webpack-bundle-analyzer build/bundle-stats.json

I noticed that the node-modules bundle is now 3x larger (from ~500 kB to 1.4 MB).

Here's my Ionic 5 app bundle:

ionic5

And here's the Ionic 6 bundle:

ionic6-rc3

The only changes between the code for Ionic v5 and v6 are a few lines to update a datetime component, so these builds are basically identical except for the Ionic version.

Expected Behavior

When upgrading to the next major Ionic version, the bundle size should be roughly the same or smaller than the previous version.

I read in the beta announcement that even smaller bundles are coming to Vue first, and then React and Angular, but I didn't except a massive size increase.

Steps to Reproduce

Update a moderately sized app from v5 to v6.

Code Reproduction URL

No response

Ionic Info

Ionic:

   Ionic CLI       : 6.18.0 (/home/me/.local/lib/node_modules/@ionic/cli)
   Ionic Framework : @ionic/react 6.0.0-rc.3

Capacitor:

   Capacitor CLI      : 3.3.2
   @capacitor/android : 3.3.2
   @capacitor/core    : 3.3.2
   @capacitor/ios     : 3.3.2

Utility:

   cordova-res : 0.15.3
   native-run  : 1.5.0

System:

   NodeJS : v14.16.0 (/usr/local/bin/node)
   npm    : 7.19.1
   OS     : Linux 4.19

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Nov 27, 2021
@sean-perkins sean-perkins added package: react @ionic/react package type: bug a confirmed bug report v6 issues specific to Framework v6 labels Nov 29, 2021
@ionitron-bot ionitron-bot bot removed the triage label Nov 29, 2021
@sean-perkins
Copy link
Contributor

@ptmkenny can you provide a sample repository reproducing this issue?

On an initial look, it doesn't appear the build output is a production build, where the treeshaking happens.

@sean-perkins sean-perkins added ionitron: needs reproduction a code reproduction is needed from the issue author and removed type: bug a confirmed bug report labels Nov 30, 2021
@ionitron-bot
Copy link

ionitron-bot bot commented Nov 30, 2021

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Ionic starter application and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

@sean-perkins sean-perkins removed the ionitron: needs reproduction a code reproduction is needed from the issue author label Nov 30, 2021
@sean-perkins
Copy link
Contributor

We've created a few examples locally with Vue and React and have been able to identify a relationship between Stencil version upgrades and bundle size increases.

@liamdebeasi
Copy link
Contributor

Thanks! We identified two issues that are causing treeshaking to not behave as expected.

  1. The framework wrappers we use from Stencil had a sideeffect that caused all components to get pulled in. This was addressed in fix(angular, react, vue): use defineCustomElement import to improve treeshaking stencil-ds-output-targets#208 and a fix will be in an upcoming release of Ionic 6.
  2. There is a potential side effect caused by the proxyCustomElement function in Stencil that only affects Webpack 4 (Webpack 5 seems to work fine). This one requires a bit more investigation, but we are working closely with the Stencil team to fix this.

Treeshaking should be fixed for Webpack 5, Vite, Rollup, ES Build, etc users in the next release of Ionic 6. It should be slightly better for Webpack 4 users, but that proxyCustomElement issue will need to be resolved before treeshaking can work as intended. I will keep this open until that second issue is resolved. Thanks!

@liamdebeasi liamdebeasi changed the title bug: bundle size triples when upgrading from react v5 to v6-rc3 bug: side effects with proxyCustomElement in stencil Dec 6, 2021
@liamdebeasi liamdebeasi added the type: bug a confirmed bug report label Dec 6, 2021
@piotr-cz
Copy link

piotr-cz commented Dec 8, 2021

@liamdebeasi

fix will be in an upcoming release of Ionic 6.

It's not included in the 6.0.0-rc.4, isn't it?
I'm asking because there is very small size decrease for my generated bundle after update (4.43 -> 4.38MB).

I've noticed significant bundle size increase after Swiper has been added in 6.0.0-rc.3 but this is probably a general problem with tree shaking when using Vite.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Dec 8, 2021

It is included in 6.0.0-rc.4. As I mentioned in #24280 (comment), you should only notice a small improvement at the moment because of the lingering proxyCustomElement side effect inside of Stencil that we are still investigating. Interestingly, this only impacts Webpack as Vite/Rollup/ESBuild are not impacted, so I am wondering if this is a bug in Webpack.

@piotr-cz
Copy link

piotr-cz commented Dec 8, 2021

@liamdebeasi Thank you for explaining

@ptmkenny
Copy link
Author

Thanks for the updates!

@liamdebeasi I updated to Ionic 6.0 official, and since you said Webpack 5 seems to be working, based on the CRA 5 alpha discussion, I updated my Ionic app to CRA5 alpha:

"react-scripts": "5.0.0-next.58+657739fb",

My app builds, and I can use it, but the bundle size is still huge (about 3x what it used to be):

ionic6-cra5-bundle-size

This is building with CRA5 alpha, which I understand is now using Webpack 5 under the hood.

@liamdebeasi
Copy link
Contributor

Darn, seems like Webpack 5 is still impacted. Thanks for the additional info!

@liamdebeasi liamdebeasi changed the title bug: side effects with proxyCustomElement in stencil bug: side effects with reassignment in ionic 6 + webpack Dec 14, 2021
@liamdebeasi
Copy link
Contributor

liamdebeasi commented Dec 14, 2021

Following up on this: This issue is happening due to a bug in Webpack. I have reported the issue here: webpack/webpack#14963

We are investigating potential workarounds and will follow up here when we have more to share.

edit Jan 27 2022: This also impacts Vite. See #24655. We have an experimental fix that seems to resolve this issue. I will update this thread when I have more to share.

@liamdebeasi liamdebeasi changed the title bug: side effects with reassignment in ionic 6 + webpack bug: side effects with reassignment in ionic 6 + webpack, vite Jan 27, 2022
@darrylnoakes
Copy link

Any updates? I noticed while looking at some bundle stuff that my bundle is now much larger since migrating from Ionic 5 + Webpack to Ionic 6 + Vite. Partly because ~30% (341kB uncompressed) of the vendor bundle is related to ion-slides and Swiper. A Swiper bundle (94kB uncompressed) is generated as well, which would be asynchronously loaded by the vendor bundle if needed.

@liamdebeasi
Copy link
Contributor

The fix for this was merged into the Stencil codebase recently: ionic-team/stencil@5dccc85

We are waiting on a stable Stencil release before updating Ionic.

@darrylnoakes
Copy link

darrylnoakes commented Feb 28, 2022

I noticed the change between Ionic 6.0.2 and 6.0.8.

Edit: See new comment

@darrylnoakes
Copy link

darrylnoakes commented Feb 28, 2022

It seems to be a change in Vite that changed the sizes.
I tried with some different combos.

Vite 2.6.10 + Ionic 6.0.2

dist/assets/index.58199c5e.js           11.12 KiB / gzip: 4.34 KiB
dist/assets/index.ba26cccb.css          27.39 KiB / gzip: 5.16 KiB
dist/assets/vendor.f8c31942.js          547.73 KiB / gzip: 137.93 KiB

Vite 2.6.10 + Ionic 6.0.8

dist/assets/index.80267744.js           11.12 KiB / gzip: 4.35 KiB
dist/assets/index.278183c8.css          27.40 KiB / gzip: 5.16 KiB
dist/assets/vendor.cf3e1419.js          545.99 KiB / gzip: 137.97 KiB

Vite 2.8.4 + Ionic 6.0.2

dist/assets/index.4a8c4790.js           11.12 KiB / gzip: 4.35 KiB
dist/assets/index.ba26cccb.css          27.39 KiB / gzip: 5.16 KiB
dist/assets/swiper.bundle.6deadf64.js   93.85 KiB / gzip: 25.25 KiB
dist/assets/vendor.5d1d29a2.js          1022.79 KiB / gzip: 215.34 KiB

Vite 2.8.4 + Ionic 6.0.8

dist/assets/index.259c3947.js           11.12 KiB / gzip: 4.35 KiB
dist/assets/index.278183c8.css          27.40 KiB / gzip: 5.16 KiB
dist/assets/swiper.bundle.6deadf64.js   93.85 KiB / gzip: 25.25 KiB
dist/assets/vendor.836811bc.js          1019.46 KiB / gzip: 215.27 KiB

@darrylnoakes
Copy link

The change happens in Vite between 2.6.14 (latest 2.6.x) and 2.7.0.

@darrylnoakes
Copy link

Relevant change is in Vite 2.7.0-beta.5.

Specifically, these changes in packages/vite/src/node/plugins/resolve.ts:

- // check for deep import, e.g. "my-lib/foo"
- const deepMatch = nestedPath.match(deepImportRE)
-
- const pkgId = deepMatch ? deepMatch[1] || deepMatch[2] : nestedPath
+ const possiblePkgIds: string[] = []
+  for (let prevSlashIndex = -1; ; ) {
+   let slashIndex = nestedPath.indexOf('/', prevSlashIndex + 1)
+   if (slashIndex < 0) {
+     slashIndex = nestedPath.length
+   }
+
+   const part = nestedPath.slice(
+     prevSlashIndex + 1,
+     (prevSlashIndex = slashIndex)
+   )
+   if (!part) {
+      break
+    }
+
+   // Assume path parts with an extension are not package roots, except for the
+   // first path part (since periods are sadly allowed in package names).
+   // At the same time, skip the first path part if it begins with "@"
+   // (since "@foo/bar" should be treated as the top-level path).
+  if (possiblePkgIds.length ? path.extname(part) : part[0] === '@') {
+     continue
+  }
+
+   const possiblePkgId = nestedPath.slice(0, slashIndex)
+   possiblePkgIds.push(possiblePkgId)
+ }
- const pkg = resolvePackageData(pkgId, basedir, options.preserveSymlinks)
+ const pkgId = possiblePkgIds.reverse().find((pkgId) => {
+   pkg = resolvePackageData(pkgId, basedir, options.preserveSymlinks)
+   return pkg
+ })

Removing that reverse() (see second diff) fixes it...
Causes these changes:

normal -> reversed

@ionic/core ->   @ionic/core/components
@stencil/core -> @stencil/core/internal/app-data
@stencil/core -> @stencil/core/internal/client
ionicons ->      ionicons/components
ionicons ->      ionicons/icons

@darrylnoakes
Copy link

darrylnoakes commented Mar 1, 2022

Via some more modding of Vite's guts to choose a hardcoded package name for these cases, I've narrowed it down to the change between @ionic/core and @ionic/core/components.

@liamdebeasi liamdebeasi added type: bug a confirmed bug report and removed type: bug a confirmed bug report labels Mar 7, 2022
@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #24895, and a fix will be available in an upcoming release of Ionic Framework.

There are two issues being discussed in this thread:

  1. There was a bug in Stencil where bundle sizes were larger than normal due to an extra reassignment. The linked PR fixes this issue in Ionic by updating our Stencil dependency to 2.14.1. This should result in a noticeable reduction in bundle size for both Vite and Webpack users.
  2. There is a change in Vite that is causing Swiper.js + ion-slides to get bundled with applications, even if they are not being used. This was noted in bug: side effects with reassignment in ionic 6 + webpack, vite #24280 (comment).

The linked PR does not do anything to address issue 2 as this is due to a change in Vite, not Ionic. The good news is that this Swiper code is being removed in the next major release of Ionic, so this issue should go away on its own. For now, you may want to consider staying with Vite 2.6.x. Alternatively, you could file an issue on the Vite repo to see if the Vite team is willing to work on a fix.

I am going to close this issue. Thanks!

@ionitron-bot
Copy link

ionitron-bot bot commented Apr 6, 2022

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Apr 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: react @ionic/react package type: bug a confirmed bug report v6 issues specific to Framework v6
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants