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

Bump Vue to 3.5.13 #13085

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

rak-phillip
Copy link
Member

@rak-phillip rak-phillip commented Jan 13, 2025

Summary

This bumps Vue and related project dependencies to versions that are compatible with 3.5.13.

Fixes #13084

Occurred changes and/or fixed issues

  • Bump Vue and related dependencies to versions compatible with 3.5.13
  • Resolve error in routes as a result of the bump
  • Resolve issues with Vue type augments as a result of the bump

Areas or cases that should be tested

  • Basic regression testing of existing features and build processes

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

declare module '*.vue' {
import Vue from 'vue';
export default Vue;
export {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This empty export is in line with guidance from the Vue docs1 on the topic of augmenting vue types.

Footnotes

  1. https://vuejs.org/guide/typescript/options-api#type-augmentation-placement

}

declare module '@vue/runtime-core' {
declare module 'vue' {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplicated/nested index name was causing errors in vue-router:

Error: A route named "index" has been added as a child of a route with the same name. Route names must be unique and a nested route cannot use the same name as an ancestor.

I understand that the child with with the same name of index is the default route, so removing this parent name should be safe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I was wondering that part.

Copy link
Member

@cnotv cnotv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some version downgraded, mismatching or not pinned, is that intended?

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
pkg/rancher-components/package.json Outdated Show resolved Hide resolved
pkg/rancher-components/package.json Show resolved Hide resolved
shell/package.json Outdated Show resolved Hide resolved
@rak-phillip
Copy link
Member Author

rak-phillip commented Jan 13, 2025

Marking this as draft as I work through the remaining issues with this PR.

  1. Several e2e tests failing - confirming that the failures are related to this change
    a. Confirmed failing e2e tests are directly related to a backend change Pin Rancher version to v2.11-2053ce644a31cd8053d1f58e2487154b0b8513b6-head for e2e tests #13111

  2. A unit test fails and needs to be fixed
    a. Confirmed failing unit test is a legitimate failure AKS network dropdown contains invalid entries & styling issues when "None" is selected #13122

  3. There are eleven new compiler warnings that appear to be related to table structure
    a. These compiler warnings are legitimate - the issues described are pre-existing in Dashboard and can be addressed separately Resolve vue compiler warnings after bump to Vue 3.5 #13124

     WARNING  Compiled with 11 warnings                                           2:25:39 PM
    
      warning  in ./pkg/aks/components/AksNodePool.vue?vue&type=template&id=65907345&scoped=true&ts=true
     
     Module Warning (from ./node_modules/vue-loader/dist/templateLoader.js):
     <tr> cannot be child of <table>, according to HTML specifications. This can cause hydration errors or potentially disrupt future functionality.
     388|            class="taints"
     389|          >
     390|            <tr>
        |            ^^^^
     391|              <th>
        |  ^^^^^^^^^^^^^^^^
     392|                <label class="text-label">
        |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     393|                  {{ t('aks.nodePools.taints.key') }}
        |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     394|                  <span class="text-error">*</span>
        |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     395|                </label>
        |  ^^^^^^^^^^^^^^^^^^^^^^
     396|              </th>
        |  ^^^^^^^^^^^^^^^^^
     397|              <th>
        |  ^^^^^^^^^^^^^^^^
     398|                <label class="text-label">
        |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     399|                  {{ t('aks.nodePools.taints.value') }}
        |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     400|                  <span class="text-error">*</span>
        |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     401|                </label>
        |  ^^^^^^^^^^^^^^^^^^^^^^
     402|              </th>
        |  ^^^^^^^^^^^^^^^^^
     403|              <th>
        |  ^^^^^^^^^^^^^^^^
     404|                <label class="text-label">
        |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     405|                  {{ t('aks.nodePools.taints.effect') }}
        |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     406|                </label>
        |  ^^^^^^^^^^^^^^^^^^^^^^
     407|              </th>
        |  ^^^^^^^^^^^^^^^^^
     408|              <th />
        |  ^^^^^^^^^^^^^^^^^^
     409|            </tr>
        |  ^^^^^^^^^^^^^^^
    

@rak-phillip rak-phillip marked this pull request as draft January 13, 2025 21:51
@rak-phillip
Copy link
Member Author

rak-phillip commented Jan 14, 2025

Regarding the failing e2e tests, these are the test results when running locally on my workstation:

  • explorer/apps/repositories.spec.ts: Repo Refresh results in correct api requests
  • workloads/daemonsets.spec.ts: modifying "Scaling and Upgrade Policy" to "On Delete" should use the correct property "OnDelete"
  • workloads/deployments.spec.ts
    • ✅ Should be able to scale the number of pods
    • ✅ Should be able to view and edit configuration of pod volumes with no custom component
    • ✅ should be able to add and remove container volume mounts
  • machine-deployments.spec.ts: can edit a MachineDeployments
  • machine-sets.spec.ts: can edit a MachineSet
  • machines.spec.ts: can edit a Machine
  • repositories.spec.ts
    • ✅ can edit a repository
    • ✅ can clone a repository
    • ✅ can download YAML
    • ✅ can refresh a repository
    • ✅ can delete a repository
    • ✅ can create a repository with basic auth
    • ✅ can create a repository with SSH key
    • ✅ can delete repositories via bulk actions
    • ✅ can create an oci repository with basic auth

⚠️ NOTE: The first run of this test failed. Subsequent runs pass

  • ⚠️ pages/explorer2/workloads/deployments.spec.ts
    • ⚠️ Should be able to scale the number of pods
    • ✅ Should be able to view and edit configuration of pod volumes with no custom component
    • ✅ should be able to add and remove container volume mounts

🗒️ Edit

Confirmed that the failing e2e tests are caused by the Rancher backend version in #13111

@rak-phillip rak-phillip force-pushed the chore/bump-vue branch 3 times, most recently from 34813bb to 7423367 Compare January 15, 2025 17:38
@rak-phillip rak-phillip changed the title Chore/bump vue Bump Vue to 3.5.13 Jan 15, 2025
@rak-phillip rak-phillip requested a review from cnotv January 15, 2025 18:23
@rak-phillip rak-phillip marked this pull request as ready for review January 15, 2025 18:23
@rak-phillip
Copy link
Member Author

rak-phillip commented Jan 15, 2025

@cnotv @codyrancher @aalves08 I think that this vue bump is in a state that is ready for review.

@aalves08 there is an issue related to dayjs and the NeuVector extension. I can't say that I understand how this change could cause the failure.. Are you able to provide any insight on this?

@aalves08
Copy link
Member

aalves08 commented Jan 16, 2025

@cnotv @codyrancher @aalves08 I think that this vue bump is in a state that is ready for review.

@aalves08 there is an issue related to dayjs and the NeuVector extension. I can't say that I understand how this change could cause the failure.. Are you able to provide any insight on this?

So, I did a bit of investigation and turns out that neuvector is using dayjs coming from shell (meaning that there is no mention on their package.json), as per https://github.com/neuvector/manager-ext/blob/main/package.json

their yarn.lock have several mentions on dayjs but they are mainly as subdependencies: https://github.com/neuvector/manager-ext/blob/main/yarn.lock

I tried to replicate this issue locally by yarn linking shell from this PR to neuvector (don't forget to run ./shell/scripts/typegen.sh first on dashboard before yarn linking shell!) but I couldn't reproduce it.

neuvector's version in branch main, when doing yarn install, uses dayjs 1.11.13.

from a "normal" shell (from dashboard's current master), it installs dayjs 1.8.29. From this PR, we also get dayjs 1.8.29, so that doesn't seem to be the problem.

All in all, I don't really understand where the problem really lies. I think that adding a resolution to neuvector might go a long way to fix this, but I've also noted another problem: neuvector's main branch is not their latest code 😱

Screenshot 2025-01-16 at 10 19 55

They do it a bit differently, where they create branches per version, meaning that branch 2.1.1 might have the most updated code on their repo. That subverts what we do in the check-plugins-build check 😩

I would just say to open an issue on their repo, talking about adding a resolution to their extension, but also mentioning that they need to keep main up-to-date, otherwise the tests that we have for extensions are meaningless on their context. We can't change the branch we use every time they update their extension... No other "official" extension does that. They all keep main as the latest code. After that, ping me with the issue number, so that I can put on slack on a channel specific for the neuvector extension.

For now I would just comment out the test for their extension (also create an issue with label area/extensions in dashboard so that we can re-enable it in the future - we don't forget about it) https://github.com/rancher/dashboard/blob/master/shell/scripts/test-plugins-build.sh#L239 and move forward if it's important for this change to go in.

@cnotv
Copy link
Member

cnotv commented Jan 16, 2025

Maybe they can do what we proposed for Rancher a time ago, to have another branch for bookkeeping as stable.
A common configuration is development, release-x.x.x, and master, where this last one is stable.
Since we do not want to mess with their branching, maybe you can ask them to use another branch and call it as they want (qa, stable, or whatever) so you could add the fixed name there.

@cnotv
Copy link
Member

cnotv commented Jan 16, 2025

Could we not just install dayjs version 1.11.13 and fix it in the shell? I think it is better to have the latest version, which is also the goal of this PR.

Signed-off-by: Phillip Rak <[email protected]>
```
Error: A route named "index" has been added as a child of a route with the same name. Route names must be unique and a nested route cannot use the same name as an ancestor.
```

Signed-off-by: Phillip Rak <[email protected]>
This updates the augment from `@vue/runtime core` to `vue`.

See: vuejs/router#2295

Signed-off-by: Phillip Rak <[email protected]>
Signed-off-by: Phillip Rak <[email protected]>
The test failure associated with the test "should display subnets grouped by network in the virtual network dropdown" is legitimate and reproducible in master - it is just that the unit test was not detecting this error. I suspect that bumping Vue and related dependencies has patched a bug in test utils.

Signed-off-by: Phillip Rak <[email protected]>
Neuvector builds currently fail with the error

```
  ERROR  Failed to compile with 1 error5:49:49 PM

 error  in utils/common.ts:85:23

TS2694: Namespace 'dayjs' has no exported member 'ManipulateType'.
    83 |   base: string,
    84 |   interval: number,
  > 85 |   intervalUnit: dayjs.ManipulateType,
       |                       ^^^^^^^^^^^^^^
    86 |   pattern = "YYYYMMDDHHmmss"
    87 | ) {
    88 |   // base format: "YYYYMMDDHHmmss"
```

It appears that neuvector might require some updates that are unrelated to bumping Vue to 3.5.x. We will disable this test until neuvector can be patched.

Signed-off-by: Phillip Rak <[email protected]>
@rak-phillip
Copy link
Member Author

rak-phillip commented Jan 16, 2025

@aalves08 I commented out the neuvector plugin test, and now capi fails with the following error

ERROR  Failed to compile with 1 error9:38:42 PM

 error  in ./index.ts

Syntax Error: Thread Loader (Worker 0)
[BABEL] /home/runner/work/dashboard/dashboard/capi-ui-extension/pkg/capi/index.ts: decode_js.EntityDecoder is not a constructor (While processing: "/home/runner/work/dashboard/dashboard/capi-ui-extension/node_modules/@vue/cli-plugin-babel/preset.js")
    at Generator.next (<anonymous>)
    at Generator.next (<anonymous>)
    at Generator.next (<anonymous>)

decode_js.EntityDecoder is not a constructor leads me to believe that there are two different versions of Vue present in node_modules during the test. Can we schedule some time to sync to work through this? The relationship between plugins and changes in the current PR for these tests isn't 100% clear to me.

edit: see also

@rak-phillip
Copy link
Member Author

Could we not just install dayjs version 1.11.13 and fix it in the shell? I think it is better to have the latest version, which is also the goal of this PR.

This could be an option as well. I would prefer to keep this change as focused on Vue as possible, but I'm willing to bump other dependencies if we find that to be required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Vue to 3.5.x
3 participants