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 match name from rejection route #304

Merged
merged 1 commit into from
Nov 10, 2024
Merged

Conversation

stackoverfloweth
Copy link
Contributor

when testing the usability of route.matches, I came up with the following test case

import { Url, isUrl, useRoute, useRouter } from '@kitbag/router';
import { computed } from 'vue';

const router = useRouter()
const route = useRoute()

function isRouteName(name: string | undefined): name is typeof route['name'] {
  return !!name && name !== 'NotFound'
}

const matches = computed(() => {
  return route.matches.reduce<Url[]>((matches, match) => {
    if(isRouteName(match.name)){
      const url = router.resolve(match.name, route.params)

      if(isUrl(url)){
        matches.push(url)
      }
    }

    return matches
  }, [])
})

You'll notice how my isRouteName had to check that the name existed AND was not the rejection route. This feels wrong, and would not continue to work if additional rejection types were added.

Instead, this PR removes the name property from the auto-generated route generated for a rejection type. My expectation is that this will have no effect on the router functionality but should make it easier for end users to validate a given route in matches.

Copy link

netlify bot commented Nov 9, 2024

Deploy Preview for kitbag-router ready!

Name Link
🔨 Latest commit 49cb688
🔍 Latest deploy log https://app.netlify.com/sites/kitbag-router/deploys/672fc985e4e35f0008e7871a
😎 Deploy Preview https://deploy-preview-304--kitbag-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pleek91
Copy link
Contributor

pleek91 commented Nov 9, 2024

@stackoverfloweth i like the direction here. Do you think users will ever need to know what the rejection is that is currently being shown? That was the intent behind the name. But I agree that's not a good solution.

@stackoverfloweth
Copy link
Contributor Author

@stackoverfloweth i like the direction here. Do you think users will ever need to know what the rejection is that is currently being shown? That was the intent behind the name. But I agree that's not a good solution.

The ResolvedRoute still has the name of the rejection type, that's used to actually build out little Rejection component view

@stackoverfloweth stackoverfloweth merged commit 3331ed0 into main Nov 10, 2024
7 checks passed
@stackoverfloweth stackoverfloweth deleted the rejection-route-name branch November 10, 2024 00:16
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.

2 participants