-
Notifications
You must be signed in to change notification settings - Fork 1
refactor(swiper): update to Swiper 9 #1
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have time/in a separate PR, could we also update the JS framework versions to the latest (Angular 15, React 18, Vue 3.2)? That way we can easily update this to Ionic 7 when it launches. (If you don't have time let me know and I can make a ticket in our backlog)
The code looks great though! I tested each app and it works well. Thanks for taking care of this!
@@ -15,10 +15,10 @@ | |||
"@capacitor/core": "3.2.5", | |||
"@capacitor/haptics": "1.1.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is related to the PR, but I get a peer dependency issue on Node 16/npm 8.19.3. Do you get the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't 🤔 Tried deleting node_modules
and re-installing (specifically in the Vue app) and everything worked okay. Using Node 16.13.0 and npm 8.1.0. I could try updating my npm if you're still seeing it after a clean reinstall?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting this:
npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR!
npm ERR! While resolving: ts-jest@27.0.7
npm ERR! Found: @types/jest@24.9.1
npm ERR! node_modules/@types/jest
npm ERR! dev @types/jest@"^24.0.19" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peerOptional @types/jest@"^27.0.0" from ts-jest@27.0.7
npm ERR! node_modules/ts-jest
npm ERR! dev ts-jest@"^27.0.4" from the root project
npm ERR! peerOptional ts-jest@"^27.0.4" from @vue/cli-plugin-unit-jest@5.0.0-beta.7
npm ERR! node_modules/@vue/cli-plugin-unit-jest
npm ERR! dev @vue/cli-plugin-unit-jest@"~5.0.0-beta.7" from the root project
npm ERR!
npm ERR! Conflicting peer dependency: @types/jest@27.5.2
npm ERR! node_modules/@types/jest
npm ERR! peerOptional @types/jest@"^27.0.0" from ts-jest@27.0.7
npm ERR! node_modules/ts-jest
npm ERR! dev ts-jest@"^27.0.4" from the root project
npm ERR! peerOptional ts-jest@"^27.0.4" from @vue/cli-plugin-unit-jest@5.0.0-beta.7
npm ERR! node_modules/@vue/cli-plugin-unit-jest
npm ERR! dev @vue/cli-plugin-unit-jest@"~5.0.0-beta.7" from the root project
npm ERR!
It looks like these are still using Vue CLI 5 beta dependencies which could explain it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to the same version of npm as you and I'm now getting the same error. Looking into it 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like npm install --legacy-peer-deps
works okay, if we're alright with that resolution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine for the purposes of getting this PR merged, but we'll want to fix this in a future PR since the peer dependencies are incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'll make a ticket to address it 👍
I'm not confident I'd have time to update the frameworks, especially Angular where we're crossing three major versions 😆 I'd happily take a look after I get back though, either that week or the next sprint depending on whether my other tasks go smoothly. |
No problem 👍 I'll make a ticket. |
Updates all three frameworks to use Swiper 9, following the same recommendations as the migration guide updates in the above PR. For React and Vue, this just meant updating the packages since we decided to stick with the framework components for now. For Angular, though, everything had to be switched over to Swiper Element.
This PR also updates all three frameworks to the latest Ionic v6 to get the fix from ionic-team/ionic-framework#26935 incorporated.