search

ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.93k stars 13.52k forks source link

feat: upgrade to swiper 5 #20033

Closed chriswep closed 4 years ago

chriswep commented 4 years ago

Bug Report

Ionic version: [x] 4.11.2

Current behavior: i have those errors being generated on users devices:

undefined is not an object (evaluating 's.$imageEl.transition') 
undefined is not an object (evaluating 't.$imageEl.transform')

they seem to be generated from here:

in both cases it can't be ensured that $imageEl is defined.

See also the issue on the swiper repo: https://github.com/nolimits4web/swiper/issues/3275

liamdebeasi commented 4 years ago

Thanks for the issue. Can you provide a code reproduction of the issue?

chriswep commented 4 years ago

@liamdebeasi i didn't manage to reproduce it, those errors come from production error logs. however looking at the code lines i posted it seems apparent to me where the errors come from ($imageEl can be undefined in both cases and needs to be guarded). Since this is an old swiper version and the library is bundled directly with ionic it's probably best to just change those lines in the ionic repo.

liamdebeasi commented 4 years ago

Makes sense. Thanks!

ionitron-bot[bot] commented 4 years ago

This issue has been labeled as help wanted. This label is added to issues that we believe would be good for contributors.

If you'd like to work on this issue, please comment here letting us know that you would like to submit a pull request for it. This helps us to keep track of the pull request and make sure there isn't duplicated effort.

For a guide on how to create a pull request and test this project locally to see your changes, see our contributing documentation.

Thank you!

chriswep commented 4 years ago

@liamdebeasi i made the fix to swiper.bundle.js but then realised that this file is re-created by an ionic build script (swiper.rollup.config.js) from the original swiper@4.5.1 package. So there either an update needs to be made to the next major swiper version (which probably would be a major issue?) or it seems to me the swiper build script (which regenerates the checked in swiper.bundle.js) should be disabled, since it's unlikely that this old version will be fixed (the author also seems to indicate that on the issue on the original repo).

What do you think?

eamador commented 4 years ago

@chriswep then shouldn't this be fixed in swiper first and then upgrade it in ionic?

It is reported, as you know, in the swiper repo too https://github.com/nolimits4web/swiper/issues/3275 but it is closed due inactivity.

I am having the error (from onGestureStart) using Swiper 4.4.6 (from @ionic/angular 4.7.4)

chriswep commented 4 years ago

@eamador sure that would be the right thing to do. However version 4 is deprecated and the author does not intend to work on the old version (which makes sense for this kind of project ..?). I'm also not sure why ionic decided to include the bundled coded directly into its repository.. Anyway, as i said above, Ionic should either upgrade to version 5 (and fix the issue in swiper if it's still there in this version) or "freeze" the code in the repo (by disabling the build script that overwrites it from the swiper-package) and fix it there.

any news @liamdebeasi ? The issue appears randomly in certain edge cases (probably the user interacting with the slider in the exact (wrong) moment) - i (and others) see it regularly in remote logs, it now also happened one time for me locally.

chriswep commented 4 years ago

@liamdebeasi bringing this up again after the ionic 5 release since the outdated swiper 4.5.1 is still used there. Any plans how to deal with this? As i explained earlier, the bundled version can be fixed in ionic however the build script that regenerates the bundle would need to be disabled then. Last time i checked the current swiper version still has the bug, so another route would be to submit a PR there however that would entail updating ionic to swiper 5.

liamdebeasi commented 4 years ago

I discussed this with the team today, and we decided to upgrade to Swiper 5 in an upcoming feature release of Ionic Framework. We are currently focused on getting bug fixes out to support the Ionic Framework 5 release, but I did create a dev build with the latest Swiper. Can people try it out in their apps and let me know if they run into any issues? Thanks!

Ionic Angular npm i @ionic/angular@5.1.0-dev.202002241552.7b5c63a

Ionic React npm i @ionic/react@5.1.0-dev.202002241552.7b5c63a

josh-m-sharpe commented 4 years ago

I'm also seeing this from production.

@chriswep any guesses at what the user's experience is when they hit this bug?

chriswep commented 4 years ago

@chriswep any guesses at what the user's experience is when they hit this bug?

@josh-m-sharpe i'm not 100% sure, however as far as i understand the code, i think the user wouldn't notice.

@liamdebeasi i think upgrading to swiper 5 is the right call however please note that the bug yet has to be fixed there (https://github.com/nolimits4web/swiper/issues/3275#issuecomment-562113342)

liamdebeasi commented 4 years ago

Ok good to know that the bug still exists there. Hopefully the Swiper dev can fix the issue there.

3zzy commented 4 years ago

My project would mainly be using the Swiper so I need to use v5, so just wondering if I could use Swiper 5 without using the slide component?

Edit: Looks like there's no breaking change between 4.5.1 and the latest (https://github.com/nolimits4web/swiper/blob/master/CHANGELOG.md) so can't I just use the latest and somehow disable rollup for overwriting temporarily until ionic 5.1.0?

liamdebeasi commented 4 years ago

Hi everyone,

Here is a dev build with ion-slides using Swiper 5. I believe the issue mentioned https://github.com/ionic-team/ionic/issues/20033#issue-532110805 is still a problem, but at least we will be on a version that is maintained.

Ionic Angular npm i @ionic/angular@5.1.0-dev.202003301547.a24142e

Ionic React npm i @ionic/react@5.1.0-dev.202003301547.a24142e

Feel free to test it out, and provide any feedback if things look broken. Thanks!

liamdebeasi commented 4 years ago

@chriswep Do you happen to have a reproduction of the original issue without ion-slides? (I.e. just using regular Swiper) It might be worthwhile to open a new issue - does not seem like the developer is watching that closed thread anymore.

chriswep commented 4 years ago

@liamdebeasi unfortunately i can't reproduce the issue, i only once saw the error popping up in dev. i (and apparently others) can only say that it consistently shows up in production logs.

i haven't used slider outside of ion-slides. as it seems the issue on the original repo is also in the context of ionic.

someone already opened a new issue on the swiper repo to draw attention: https://github.com/nolimits4web/swiper/issues/3520

josh-m-sharpe commented 4 years ago

@liamdebeasi I also cannot reproduce this locally, but have active, ongoing, production occurrences of the issue. I say just merge swiper5 (we should have that anyways... right?) and I'll be the first to upgrade/test/deploy it.

liamdebeasi commented 4 years ago

Thanks for the feature request. This has been resolved via https://github.com/ionic-team/ionic/pull/20917 and will be available in an upcoming release of Ionic Framework.

Davei234 commented 4 years ago

@liamdebeasi can you do one more update to from swiper 5.3.6 to 5.3.7? It looks like this bug was actually fixed in that version: https://github.com/nolimits4web/swiper/commit/d89a73cd63f3c0b0cfc7cb88f9e3120ec3f19930

liamdebeasi commented 4 years ago

All set https://github.com/ionic-team/ionic/pull/21055.

3zzy commented 4 years ago

My app is entirely based on this component so just wondering whats the ETA for the next release?

liamdebeasi commented 4 years ago

We do not comment on exact timing for releases since they can sometimes change, but we plan to have this update out soon.

ionitron-bot[bot] commented 4 years ago

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.