Skip to content

Commit

Permalink
fix(vue): tabs and parameterized routes work with latest vue (#28846)
Browse files Browse the repository at this point in the history
Issue number: resolves #28774

---------

<!-- 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. -->

There are two issues causing Ionic Vue apps to not behave as intended
with certain versions of Vue:

1. In Vue 3.3 a [breaking change
shipped](vuejs/core#9916) that changes the
default behavior of the `watch` inside of IonRouterOutlet to be a
shallow watcher instead of a deep watcher. This caused the router outlet
to not consistent re-render. While the change was later reverted by the
Vue team, they expressed that the change [may re-land in a future minor
release](vuejs/core#9965 (comment)).
As a result, we will need to account for this inside of Ionic.
2. In Vue 3.2 a [custom elements improvement
shipped](https://github.com/vuejs/core/blob/main/changelogs/CHANGELOG-3.2.md#3238-2022-08-30)
that changed how custom elements are referred to in VNodes.

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

- The affected `watch` call now is now explicitly a deep watcher. This
change is backwards compatible as well as forward compatible with
upcoming Vue changes.
- Updated IonTabs to account for the new VNode behavior for custom
elements. Ionic still supports version of Vue that do not have this
improvement, so we need to account for both behaviors for now. I also
added a tech debt ticket to remove the old checks when we drop support
for older versions of Vue.
- Updated E2E test dependencies. During this update some of our tests
needed to be updated to account for newer versions of Vue/Vitest.
Overall I was able to simplify a lot of our tests as a result.

I plan to add renovatebot to these E2E test apps, but I will handle that
in a separate PR.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  4. Update the BREAKING.md file with the breaking change.
5. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer
for more information.
-->


## Other information

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

Dev build: `7.6.6-dev.11705526292.1bc0acb5`

Note: Both of the issues cause tests to fail when using the latest
dependencies in the Vue E2E test app. However, I need to use the latest
dependencies so I can demonstrate that my changes do fix the reported
issues. As a result, I have both fixes in the same PR.
  • Loading branch information
liamdebeasi authored Jan 19, 2024
1 parent 9262f7d commit 5bc4399
Show file tree
Hide file tree
Showing 10 changed files with 81 additions and 93 deletions.
10 changes: 9 additions & 1 deletion packages/vue/src/components/IonRouterOutlet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,15 @@ export const IonRouterOutlet = /*@__PURE__*/ defineComponent({

previousMatchedRouteRef = currentMatchedRouteRef;
previousMatchedPath = currentRoute.path;
}
},
/**
* Future versions of Vue may default watching nested
* reactive objects to "deep: false".
* We explicitly set this watcher to "deep: true" to
* account for that.
* https://github.com/vuejs/core/issues/9965#issuecomment-1875067499
*/
{ deep: true }
);

const canStart = () => {
Expand Down
35 changes: 27 additions & 8 deletions packages/vue/src/components/IonTabs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,28 @@ const DID_CHANGE = "ionTabsDidChange";

// TODO(FW-2969): types

/**
* Vue 3.2.38 fixed an issue where Web Component
* names are respected using kebab case instead of pascal case.
* As a result, we need to account for both here since we support
* versions of Vue < 3.2.38.
*/
// TODO FW-5904
const isRouterOutlet = (node: VNode) => {
return (
node.type &&
((node.type as any).name === "IonRouterOutlet" ||
node.type === "ion-router-outlet")
);
};

const isTabBar = (node: VNode) => {
return (
node.type &&
((node.type as any).name === "IonTabBar" || node.type === "ion-tab-bar")
);
};

export const IonTabs = /*@__PURE__*/ defineComponent({
name: "IonTabs",
emits: [WILL_CHANGE, DID_CHANGE],
Expand All @@ -19,9 +41,8 @@ export const IonTabs = /*@__PURE__*/ defineComponent({
* inside of ion-tabs.
*/
if (slottedContent && slottedContent.length > 0) {
routerOutlet = slottedContent.find(
(child: VNode) =>
child.type && (child.type as any).name === "IonRouterOutlet"
routerOutlet = slottedContent.find((child: VNode) =>
isRouterOutlet(child)
);
}

Expand Down Expand Up @@ -57,13 +78,11 @@ export const IonTabs = /*@__PURE__*/ defineComponent({
* since that needs to be inside of `.tabs-inner`.
*/
const filteredContent = slottedContent.filter(
(child: VNode) =>
!child.type ||
(child.type && (child.type as any).name !== "IonRouterOutlet")
(child: VNode) => !child.type || !isRouterOutlet(child)
);

const slottedTabBar = filteredContent.find(
(child: VNode) => child.type && (child.type as any).name === "IonTabBar"
const slottedTabBar = filteredContent.find((child: VNode) =>
isTabBar(child)
);
const hasTopSlotTabBar =
slottedTabBar && slottedTabBar.props?.slot === "top";
Expand Down
6 changes: 3 additions & 3 deletions packages/vue/test/apps/vue3/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
"@ionic/vue": "^6.0.12",
"@ionic/vue-router": "^6.0.12",
"ionicons": "^6.0.4",
"vue": "^3.2.31",
"vue-router": "^4.0.14"
"vue": "^3.4.14",
"vue-router": "^4.2.5"
},
"devDependencies": {
"@typescript-eslint/eslint-plugin": "^5.4.0",
Expand All @@ -35,7 +35,7 @@
"jsdom": "^20.0.0",
"typescript": "~4.5.5",
"vite": "^3.1.4",
"vitest": "^0.23.4",
"vitest": "^1.2.1",
"wait-on": "^5.3.0"
},
"engines": {
Expand Down
17 changes: 6 additions & 11 deletions packages/vue/test/base/tests/unit/hooks.spec.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,9 @@
import { mount } from '@vue/test-utils';
import { describe, expect, it } from 'vitest';
import { createRouter, createWebHistory } from '@ionic/vue-router';
import { IonicVue, IonApp, IonRouterOutlet, IonPage, useIonRouter, createAnimation } from '@ionic/vue';
import { IonicVue, IonRouterOutlet, IonPage, useIonRouter } from '@ionic/vue';
import { mockAnimation, waitForRouter } from './utils';

const App = {
components: { IonApp, IonRouterOutlet },
template: '<ion-app><ion-router-outlet /></ion-app>',
}

const BasePage = {
template: '<ion-page></ion-page>',
components: { IonPage },
Expand Down Expand Up @@ -40,7 +35,7 @@ describe('useIonRouter', () => {

router.push('/');
await router.isReady();
const wrapper = mount(App, {
const wrapper = mount(IonRouterOutlet, {
global: {
plugins: [router, IonicVue]
}
Expand Down Expand Up @@ -85,7 +80,7 @@ describe('useIonRouter', () => {

router.push('/');
await router.isReady();
const wrapper = mount(App, {
const wrapper = mount(IonRouterOutlet, {
global: {
plugins: [router, IonicVue]
}
Expand Down Expand Up @@ -137,7 +132,7 @@ describe('useIonRouter', () => {

router.push('/');
await router.isReady();
const wrapper = mount(App, {
const wrapper = mount(IonRouterOutlet, {
global: {
plugins: [router, IonicVue]
}
Expand Down Expand Up @@ -181,7 +176,7 @@ describe('useIonRouter', () => {

router.push('/');
await router.isReady();
const wrapper = mount(App, {
const wrapper = mount(IonRouterOutlet, {
global: {
plugins: [router, IonicVue]
}
Expand Down Expand Up @@ -229,7 +224,7 @@ describe('useIonRouter', () => {

router.push('/');
await router.isReady();
const wrapper = mount(App, {
const wrapper = mount(IonRouterOutlet, {
global: {
plugins: [router, IonicVue]
}
Expand Down
13 changes: 4 additions & 9 deletions packages/vue/test/base/tests/unit/lifecycle.spec.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,10 @@
import { mount } from '@vue/test-utils';
import { describe, expect, it, vi } from 'vitest';
import { createRouter, createWebHistory } from '@ionic/vue-router';
import { IonicVue, IonApp, IonRouterOutlet, IonTabs, IonPage } from '@ionic/vue';
import { IonicVue, IonRouterOutlet, IonTabs, IonPage } from '@ionic/vue';
import { defineComponent } from 'vue';
import { waitForRouter } from './utils';

const App = {
components: { IonApp, IonRouterOutlet },
template: '<ion-app><ion-router-outlet /></ion-app>',
}

const BasePage = {
template: '<ion-page :data-pageid="name"></ion-page>',
components: { IonPage },
Expand Down Expand Up @@ -54,7 +49,7 @@ describe('Lifecycle Events', () => {
// Initial render
router.push('/');
await router.isReady();
const wrapper = mount(App, {
const wrapper = mount(IonRouterOutlet, {
global: {
plugins: [router, IonicVue]
}
Expand Down Expand Up @@ -108,7 +103,7 @@ describe('Lifecycle Events', () => {
template: `
<ion-page>
<ion-tabs>
<ion-router-outlet></ion-router-outlet>
<IonRouterOutlet />
</ion-tabs>
</ion-page>
`,
Expand Down Expand Up @@ -148,7 +143,7 @@ describe('Lifecycle Events', () => {
// Initial render
router.push('/tab1');
await router.isReady();
const wrapper = mount(App, {
const wrapper = mount(IonRouterOutlet, {
global: {
plugins: [router, IonicVue]
}
Expand Down
11 changes: 3 additions & 8 deletions packages/vue/test/base/tests/unit/page.spec.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
import { mount } from '@vue/test-utils';
import { describe, expect, it } from 'vitest';
import { createRouter, createWebHistory } from '@ionic/vue-router';
import { IonicVue, IonApp, IonRouterOutlet, IonPage } from '@ionic/vue';

const App = {
components: { IonApp, IonRouterOutlet },
template: '<ion-app><ion-router-outlet /></ion-app>',
}
import { IonicVue, IonRouterOutlet, IonPage } from '@ionic/vue';

describe('IonPage', () => {
it('should add ion-page class', async () => {
Expand All @@ -25,7 +20,7 @@ describe('IonPage', () => {

router.push('/');
await router.isReady();
const wrapper = mount(App, {
const wrapper = mount(IonRouterOutlet, {
global: {
plugins: [router, IonicVue]
}
Expand All @@ -50,7 +45,7 @@ describe('IonPage', () => {

router.push('/');
await router.isReady();
const wrapper = mount(App, {
const wrapper = mount(IonRouterOutlet, {
global: {
plugins: [router, IonicVue]
}
Expand Down
12 changes: 2 additions & 10 deletions packages/vue/test/base/tests/unit/router-outlet.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,14 @@ import { afterEach, describe, expect, it, vi } from 'vitest';
import { createRouter, createWebHistory } from '@ionic/vue-router';
import {
IonicVue,
IonApp,
IonRouterOutlet,
IonPage,
useIonRouter,
createAnimation
} from '@ionic/vue';
import { onBeforeRouteLeave } from 'vue-router';
import { mockAnimation, waitForRouter } from './utils';

enableAutoUnmount(afterEach);

const App = {
components: { IonApp, IonRouterOutlet },
template: '<ion-app><ion-router-outlet /></ion-app>',
}

const BasePage = {
template: '<ion-page :data-pageid="name"></ion-page>',
components: { IonPage },
Expand Down Expand Up @@ -60,7 +52,7 @@ describe('Routing', () => {

router.push('/');
await router.isReady();
const wrapper = mount(App, {
const wrapper = mount(IonRouterOutlet, {
global: {
plugins: [router, IonicVue]
}
Expand Down Expand Up @@ -122,7 +114,7 @@ describe('Routing', () => {

router.push('/');
await router.isReady();
const wrapper = mount(App, {
const wrapper = mount(IonRouterOutlet, {
global: {
plugins: [router, IonicVue]
}
Expand Down
Loading

0 comments on commit 5bc4399

Please sign in to comment.