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

Replace nuxt-link with router-link #10716

Merged
merged 3 commits into from
Apr 1, 2024

Conversation

rak-phillip
Copy link
Member

@rak-phillip rak-phillip commented Mar 27, 2024

Summary

This removes the NuxtLink & NLink components from Dashboard. If an extension or outside component attempts to use NuxtLink or NLink, a deprecation warning should print to the console and router-link will be used instead.

Technical notes summary

I'm attempting to make this change in a way that will be more backwards compatible with extensions that might relay on NuxtLink or NLink. This is done by replacing the NuxtLink component with an alias that will print a deprecation warning in the console.

Areas or cases that should be tested

This affects all routing throughout dashboard.

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

`nuxt-link` will be removed from Dashboard and will print a deprecation warning when it is used.

Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
@rak-phillip rak-phillip added this to the v2.9.0 milestone Mar 27, 2024
@rak-phillip rak-phillip self-assigned this Mar 27, 2024
@rak-phillip
Copy link
Member Author

@torchiaf I'm requesting a review from you so that you can test with Harvester. I want to avoid another situation where we might need to revert a portion of this PR like we did in #10676

@rak-phillip rak-phillip changed the title Chore/remove nuxt link Replace nuxt-link with router-link Mar 27, 2024
codyrancher
codyrancher previously approved these changes Mar 27, 2024
Copy link
Contributor

@codyrancher codyrancher left a comment

Choose a reason for hiding this comment

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

As long as tests pass this LGTM.

Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Copy link
Contributor

@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 think you covered everything, but please wait for @torchiaf about Harvester, otherwise this will get reverted anyway.

LGTM

Copy link
Member

@torchiaf torchiaf left a comment

Choose a reason for hiding this comment

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

Harvester router seems to be working correctly.
If possible, I would remove the warning message when loading Harvester.
LGTM

@rak-phillip
Copy link
Member Author

If possible, I would remove the warning message when loading Harvester.

@torchiaf can you expand on why we would want to make an exception for Harvester? If we are flagging features as deprecated in v2.9 of Dashboard, would we not want to warn all extension authors that this will break in later versions after said features are removed?

@torchiaf
Copy link
Member

torchiaf commented Mar 29, 2024

If possible, I would remove the warning message when loading Harvester.

@torchiaf can you expand on why we would want to make an exception for Harvester? If we are flagging features as deprecated in v2.9 of Dashboard, would we not want to warn all extension authors that this will break in later versions after said features are removed?

In general, I think you are right, but in the Harvester's case, those warnings would be visible only when using Rancher + Harvester older versions in embedded mode.
We will have to port this code in the next Harvester version, including the warning logic - in theory the Harvester plugin would take care of showing the warnings then (even in standalone mode).

showing the deprecation warning when updating Rancher is correct.

Anyway, this is really a non-issue I think, that's why I approved it; sorry, I was a little pedantic here 😃

@rak-phillip rak-phillip merged commit 6206f97 into rancher:master Apr 1, 2024
27 checks passed
@rak-phillip rak-phillip deleted the chore/remove-nuxt-link branch April 1, 2024 15:00
@cnotv cnotv mentioned this pull request Apr 4, 2024
@cnotv cnotv mentioned this pull request Sep 19, 2024
7 tasks
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.

4 participants