-
Notifications
You must be signed in to change notification settings - Fork 257
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
FIX: Vue3 => Error info when creating NavLink is not shown to user #11956
base: master
Are you sure you want to change the base?
FIX: Vue3 => Error info when creating NavLink is not shown to user #11956
Conversation
30094bb
to
c14a0c4
Compare
@richard-cox added you as a reviewer not only because I couldn't find the exact root cause but because of the implications this might have on other areas and potencial checks to be done. Let me know if you want to get together to look into this |
This looks like another instance where |
@rak-phillip I am interested in knowing more about the root cause of this.... I never understood why adding a root element would fix this 😛 |
@aalves08 this all has to do with how Vue3 manages fallthrough attributes1 for root-level ancestor components - I've created a minimal repro to demonstrate the behavior:
This can be fixed in several ways:
Footnotes |
@richard-cox I can confirm that |
c14a0c4
to
f9bc52e
Compare
@richard-cox #11971 did not fix this. Used |
hmm that gives me quite a bit of pause.. perhaps the solution implemented in #11971 is not the correct one for addressing this problem at the root. I'd like to investigate the issue here a little bit to better understand what else might be causing the issue. |
I've narrowed this one down to the dashboard/shell/mixins/create-edit-view/impl.js Lines 15 to 18 in 76147e0
This is a different manifestation of the same issue, fallthrough attributes for root-level ancestor components are overwriting props. I don't intend to block the merging of this PR and I think that the I'm also going to investigate alternative solutions to see if we can prevent this issue from popping up in other components. |
@richard-cox can this fix be approved and merged or should we wait for more feedback from @rak-phillip ? |
There's something still very odd about this. If the error is down to a a mixin containing a data The hierarchy is strange
So errors is passed around in different ways. However isn't the same everywhere... in surprising ways
I would expect errors to come from ResourceDefinition (via mixin) --> edit component --> cruresouce and be broken everywhere, which isn't happening. Basically looking for some clarification / comfort that the problem here isn't universal |
@richard-cox this is explained by fallthrough attributes overwriting props in root-level ancestor components. in In components like dashboard/shell/components/ResourceDetail/index.vue Lines 422 to 437 in 5405402
dashboard/shell/edit/ui.cattle.io.navlink.vue Lines 281 to 293 in 5405402
I think that the overall problem is two-fold:
The way that we have been addressing this issue is to add |
@rak-phillip I get the general failure, but trying to understand why it doesn't also make pages like persistentvolumes fail. Something to do with vue and the v-if/else? secrets now seem clear (given cruresource is wrapped in a root level form element) |
@richard-cox
wrt to |
Grand, that's all good now. Then we just need to run through the |
Looks like this same issue appears in a number of different places (some aren't fixed by inheritAttrs: false,) Standout - These need to be confirmed or fixed in 2.10.0
Debatable - These need to be confirmed but not maybe fixed in 2.10.0
Others - These need to be confirmed and fixed at some point
Of Note
@rak-phillip i'll leave it up to you on how to split those up, and where it leaves this PR |
@richard-cox I can work through each of the components listed and put together an issue to resolve any offenders. I don't see any reason why this PR should be blocked at this point. |
Summary
Fixes #11937
Occurred changes and/or fixed issues
Technical notes summary
Although the changes look bigger, what was needed to make it work was just adding a
div
as the outermost container to wrap theCruResource
component. No errors were being thrown, so I don't know the root cause for this.Also, it may be important to cover all of the
CruResource
usages to check if it's a general problem or not and act accordingly.Areas or cases that should be tested
Resource Search
--> type in NavLink --> click on NavLink --> CreateAAA
for the name --> Enter anything forDestination URL
--> CreateAreas which could experience regressions
Screenshot/Video
Screen.Recording.2024-09-18.at.14.35.40.mov
Checklist