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

Rename resource to resourceType in ResourceDetail #11971

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

rak-phillip
Copy link
Member

@rak-phillip rak-phillip commented Sep 19, 2024

Summary

This resolves an issue that we've been seeing in edit components where the resource data prop, being passed to dynamically rendered components via v-bind="$data", is overwriting props in a grandchild component at the root of the child. The root cause of this issue is directly attributed to how Vue3 manages fallthrough attributes12.

Fixes #

Occurred changes and/or fixed issues

  • rename resource to resourceType to prevent resource colliding with granchild props as a fallthrough attribute

Technical notes summary

I'm identifying all instances of resource: in our edit components with the following

grep -rc --exclude-dir=node_modules/ 'resource:' shell/edit/ | grep -v ":0"
shell/edit/__tests__/monitoring.coreos.com.prometheusrule.test.ts:1
shell/edit/autoscaling.horizontalpodautoscaler/metrics-row.vue:1
shell/edit/autoscaling.horizontalpodautoscaler/resource-metric.vue:1
shell/edit/fleet.cattle.io.cluster.vue:1
shell/edit/logging.banzaicloud.io.output/__tests__/logging.banzaicloud.io.output.test.ts:1
shell/edit/management.cattle.io.projectroletemplatebinding.vue:1
shell/edit/monitoring.coreos.com.receiver/index.vue:1
shell/edit/monitoring.coreos.com.route.vue:1
shell/edit/networking.k8s.io.ingress/Certificates.vue:1
shell/edit/provisioning.cattle.io.cluster/import.vue:2
shell/edit/provisioning.cattle.io.cluster/index.vue:1
shell/edit/provisioning.cattle.io.cluster/rke2.vue:5
shell/edit/workload/mixins/workload.js:1
shell/edit/management.cattle.io.project.vue:2

We have two potential alternatives to this change:

  1. Add inheritAttrs: false to each component under shell/edit to disable attribute fallthrough behavior
  2. Use either v-bind.attr="$data" or v-bind.prop="$data" to prevent the fallthrough behavior2

Areas or cases that should be tested

This change affects every component that can feasibly be rendered by `ResourceDetail.

Areas which could experience regressions

Any components with an actual resource prop that might be dynamically rendered via:

<component
:is="showComponent"
v-else
ref="comp"
v-model:value="value"
v-bind="$data"
:done-params="doneParams"
:done-route="doneRoute"
:mode="mode"
:initial-value="initialModel"
:live-value="liveModel"
:real-mode="realMode"
:class="{'flex-content': flexContent}"
@update:value="$emit('input', $event)"
@set-subtype="setSubtype"
/>

It's difficult to assert that this is a safe approach without any form of contract between components that we can dynamically render and ResourceDetail.

We would also need to assert that any mixins associated with any given component does not also declare a resource prop.

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

Footnotes

  1. https://vuejs.org/guide/components/attrs.html#fallthrough-attributes

  2. https://vuejs.org/api/built-in-directives.html#v-bind 2

@rak-phillip
Copy link
Member Author

@eva-vashkevich you've addressed a lot of regressions in edit components by adding inheritAttrs: false. Would you be willing to confirm that this change would have resolved the issue?

@rak-phillip
Copy link
Member Author

@richard-cox I'm adding you as a reviewer to get your feedback on whether or not we believe there to be too much risk associated with this change. My testing has been looking good up through this point, and I want to get a fresh perspective on this change.

@eva-vashkevich
Copy link
Member

This does seem to fix the issues I've worked on that were fixed with inheritAttrs

@rak-phillip rak-phillip marked this pull request as ready for review September 19, 2024 20:11
@rak-phillip rak-phillip added this to the v2.10.0 milestone Sep 19, 2024
@richard-cox
Copy link
Member

General approach looks good, the more eyes on this the better though.

I'd like to kick the tires a bit more before this merges

@rak-phillip
Copy link
Member Author

@aalves08 @mantis-toboggan-md adding you as reviewers so that we can more eyes on this change

@rak-phillip
Copy link
Member Author

This comment has additional information and a minimal repro to demonstrate the behavior:

#11956 (comment)

Copy link
Member

@mantis-toboggan-md mantis-toboggan-md left a comment

Choose a reason for hiding this comment

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

I haven't been able to identify anything broken by this PR but it is a tough one to verify. Would this change potentially affect components under shell/detail/ as well? I see the ResourceDetail is used in both create.vue and _id.vue pages. I've searched for references to this.resource and resource: excluding test files and the shell/list directory looking for components that either define a resource prop or otherwise expect it passed in from ResourceDetail.

The PR looks good from my perspective but I definitely agree about the need for multiple reviewers on this one.

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

Given it a prod, looks good.

I also went through the ./shell/detail components, no references to resource there (which came from above).

Would vote for one more green tick and then we can merge.

Copy link
Member

@aalves08 aalves08 left a comment

Choose a reason for hiding this comment

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

All instances seem correctly replaced as for looking at the code in the editor, so LGTM

@rak-phillip rak-phillip merged commit 7c3317d into rancher:master Sep 24, 2024
68 checks passed
@rak-phillip rak-phillip deleted the chore/resource-refactor branch September 24, 2024 15:54
torchiaf added a commit to torchiaf/harvester that referenced this pull request Sep 30, 2024
torchiaf added a commit to torchiaf/harvester that referenced this pull request Sep 30, 2024
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.

5 participants