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

Remove inline defined middleware #10956

Merged
merged 1 commit into from
May 7, 2024

Conversation

codyrancher
Copy link
Contributor

@codyrancher codyrancher commented May 6, 2024

Summary

Remove inline defined middleware. Replace it primarily with router configuration.

Areas or cases that should be tested

We already have adequate automated testing for this.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't migrate this file to the router config because it relies on the store already having schemas.

The initialization sequence (even modified slightly) will not have schemas loaded before route resolution when re/loading a page.

@codyrancher codyrancher changed the title Middle remove Remove inline defined middleware May 7, 2024
@codyrancher codyrancher added this to the v2.9.0 milestone May 7, 2024
@codyrancher codyrancher marked this pull request as ready for review May 7, 2024 20:06
@@ -3,13 +3,13 @@ import { NAME as SETTINGS } from '@shell/config/product/settings';
import { MANAGEMENT } from '@shell/config/types';

export default {
middleware({ redirect, route, store } ) {
const hasSettings = !!store.getters[`management/schemaFor`](MANAGEMENT.SETTING);
beforeCreate() {
Copy link
Member

Choose a reason for hiding this comment

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

I'll preface this comment by saying that I'm fine with the approach represented here, I'm more interested in exploring an alternative to see if it makes sense for this use case.

Have we considered utilizing in-component guards1 instead of the beforeCreate() hook? I think that the guard would better communicate what we are doing within this component, but it comes with the tradeoff that we might not have access to this when we want it..

Footnotes

  1. https://v3.router.vuejs.org/guide/advanced/navigation-guards.html#in-component-guards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't considered that. Do you know of a way to access the store from the guard?

It's actually a pretty reasonable idea which may apply better in that it executes before the component is created just like middleware.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it looks like we would have to do something like this instead

beforeRouteEnter (to, from, next) {
  next(vm => {
    const hasSettings = !!vm.$store.getters[`management/schemaFor`](MANAGEMENT.SETTING);

    ...
  })
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll keep looking into this. It's interesting and not as straight forward as I would've thought. If we access the vm it looks like the component will already be created so not a big advantage.

image

There's another suggestion I found which mentions just importing our store and accessing it directly. With responses that would resemble our current predicament in regards to how we can access our store in the app.

image

Any thoughts on the idea of making the store global?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave the global store a shot and it's a no go. The schemas aren't present when re/loaded.

I'm fine with leaving it as is or using the next(vm => {}). Do you think one is better than the other?

Copy link
Member

@rak-phillip rak-phillip May 7, 2024

Choose a reason for hiding this comment

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

I think we can leave this as is. It sounds like we won't get much of a benefit and the syntax will end up being less direct with the in-component guard.

Thanks for entertaining the idea 🙂

Copy link
Member

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

LGTM

@codyrancher codyrancher merged commit 820374c into rancher:master May 7, 2024
35 checks passed
@cnotv
Copy link
Contributor

cnotv commented May 10, 2024

Nice cleanup 👍

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.

None yet

3 participants