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

Vue3 migration script for extensions has multiple issues #12276

Open
jordojordo opened this issue Oct 16, 2024 · 0 comments · May be fixed by #12277
Open

Vue3 migration script for extensions has multiple issues #12276

jordojordo opened this issue Oct 16, 2024 · 0 comments · May be fixed by #12277
Assignees
Labels
area/extensions kind/bug QA/manual-test Indicates issue requires manually testing
Milestone

Comments

@jordojordo
Copy link
Member

jordojordo commented Oct 16, 2024

Describe the bug

There are a number of issues with the migration script for extensions found in shell/scripts/vue-migrate.js.

When running the script within the Kubewarden UI Extension, these were noted:

  • The script failed initially. On line 534 there is a forEach loop on the eslintConfig rules property. If the eslintConfig.rules object does not exist the script will fail as eslintConfig.rules[rule] will return eslintConfig.rules is undefined.

  • It changed files in the /tests directory.

    • This is outside of the scope of extensions and should be taken care of manually
    • Updated a utility file which imported Vue originally, left Vue artifacts within the file which will cause errors when attempting to run
    • Changed any mention of "Vue" to "vueApp". There is a constant created in this file called localVue = createdLocalVue() - the original constant name is the same, but subsequent method calls on this class were changed. For example:
      // <<<<< Current
      localVue.use(Vuex);
      // >>>>>
      // <<<<< Incoming
      localvueApp.use(Vuex);
      // >>>>>
        ```
  • All workflows within the ./.github/workflows/* directory have been updated, this includes ones that are not created by the @rancher/extension creator.

    • Only the original files (if found) should be updated
      • Perhaps we should just show a warning in the cli output which notifies which files should be updated or at least checked.
  • Not all of the dependencies listed in the creator are added/updated in the root level package.json

    • Do these dependencies matter? Should they be removed from the creator if not?
  • .eslintrc.js should not be modified, this is specific to each extension developer's preference.

    • We could suggest changes to this file but ultimately it should be up to the developer
  • The annotations in a pkg's package.json file were not updated

    • If these are left unmodified, this could render their extension unusable with 2.10 or shell version 3.
  • Any elements that contain a v-for within an element will have any additional attributes removed if it's a single line element.

    • Also the key will be modified from the original. For example in ./pkg/kubewarden/chart/kubewarden/admission/ContextAware/index.vue there is a div that contained a key:
      • :key="'filtered-resource-' + index"
      • This has been modified to :key="index"
    • ^ Similar behavior when an element with a v-for loop has the key property in the 1st child component. For example ./pkg/kubewarden/components/InstallWizard.vue had a li element with the key: :key="step.name + 'li'"
  • An element that contains a v-for loop that is a multi-line element will have broken syntax.

    • For example in `./pkg/kubewarden/detail/policies.kubewarden.io.policyserver.vue:
    <!-- Current -->
    <CountGauge
      v-for="(group, key) in policyGauges"
      :key="key"
      :total="relatedPoliciesTotal"
      :useful="group.count || 0"
      :graphical="false"
      :primary-color-var="`--sizzle-${group.color}`"
      :name="key"
    />
    <!-- Modified -->
    <CountGauge
      v-for="(group, key) in policyGauges" :key="key":total="relatedPoliciesTotal"
      :useful="group.count || 0"
      :graphical="false"
      :primary-color-var="`--sizzle-${group.color}`"
      :name="key"
    />
  • Any element that uses a v-for loop and has a child element that also has a v-for loop will be broken in the child component as a key must be added to the element. The nested v-for will contain the same variable for the index property, causing the error:
    - Variable 'i' is already declared in the upper scope

  • If there is a $set method that uses a dynamic value in the key argument, the additional [] will be added to the modified method.

    • For example in ./pkg/kubewarden/chart/kubewarden/policy-server/Registry/index.vue there is a set for:
      • this.$set(this.chartValues, [prop], this[prop]);
      • this has been modified to:
        • this.chartValues[[prop]] = this[prop];
    • Same goes for this.$delete, for example:
      • this.$delete(this.value, [key]);
      • modified to: delete this.value[[key]];
  • Any use of ::v-deep that used the method syntax is off. For example:

    • In ./pkg/kubewarden/components/Policies/PolicyReadmePanel.vue there is a style for:
      • ::v-deep(.btn-sm)
      • Modified to: :deep()(.btn-sm) - This returns a syntax error
  • A component that contained a key property that is not within a v-for loop has the key property deleted.

    • For example ./pkg/kubewarden/components/Questions/Array.vue
  • v-popover elements were not updated to VDropdown

    • Subsequent <template #popover> elements not updated to <template #popper>
@jordojordo jordojordo added this to the v2.10.1 milestone Oct 16, 2024
@jordojordo jordojordo self-assigned this Oct 16, 2024
@github-actions github-actions bot added the QA/dev-automation Issues that engineers have written automation around so QA doesn't have look at this label Oct 16, 2024
@jordojordo jordojordo added QA/manual-test Indicates issue requires manually testing and removed QA/dev-automation Issues that engineers have written automation around so QA doesn't have look at this labels Oct 16, 2024
@jordojordo jordojordo linked a pull request Oct 16, 2024 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/extensions kind/bug QA/manual-test Indicates issue requires manually testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant