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

GRW-1510 / improvements to middleware routing #575

Merged
merged 6 commits into from Sep 23, 2022

Conversation

KarnellSchultz
Copy link
Contributor

@KarnellSchultz KarnellSchultz commented Sep 22, 2022

Describe your changes

  • Add logging on the root middleware (this is for our testing now, could be removed later)

  • Changed the routing to use lowercase routing for pretty urls 🍒

    • the url preview url is going from .../sv-SE/ to .../sv-se/
    • Screenshot 2022-09-22 at 10 03 04
  • Tighten up the types being used by for localisation

    • Screenshot 2022-09-22 at 10 02 26

Justify why they are needed

  • Creates nicer urls which are lowercase which is inline with how we'd like to show our urls.
  • The types changes prevent users of some of the localisation functions so that users do not use the funcs in a way that would be unexpected.

Jira issue(s): GRW-1510

Checklist before requesting a review

  • I have performed a self-review of my code

@KarnellSchultz KarnellSchultz self-assigned this Sep 22, 2022
@KarnellSchultz KarnellSchultz requested a review from a team as a code owner September 22, 2022 08:09
@vercel
Copy link

vercel bot commented Sep 22, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
racoon-store ✅ Ready (Inspect) Visit Preview Sep 22, 2022 at 2:13PM (UTC)
1 Ignored Deployment
Name Status Preview Updated
racoon ⬜️ Ignored (Inspect) Sep 22, 2022 at 2:13PM (UTC)

Copy link
Contributor

@simonauner simonauner left a comment

Choose a reason for hiding this comment

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

gif

Yazzzz! More types! Type it harder!

apps/store/src/lib/l10n/types.ts Outdated Show resolved Hide resolved
@@ -60,7 +60,7 @@ export const normalizeLocale = (locale: string | undefined): string | undefined
}

// We use en-SE ISO format for settings but downcase it for routing to get nicer URLs
export const routingLocale = (locale: string) => locale.toLowerCase()
export const routingLocale = (locale: LocaleValues) => locale.toLowerCase() as RoutingLocale
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm no TS pro, but could this be or will TS complain?

Suggested change
export const routingLocale = (locale: LocaleValues) => locale.toLowerCase() as RoutingLocale
export const routingLocale = (locale: LocaleValues):RoutingLocale => locale.toLowerCase()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeahhh feels a lil dirty to be casting that type but TS complains without it since toLowerCase() only returns a string. So going in we have out clean and specific type. but coming out we only have a string :feelsbad:

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we worker around with conversion function that return proper type
asRoutingLocale = (locale: LocaleValues): RoutingLocale => { return locale.toLowerCase() as RoutingLocale }

But only ways without assertion that I know are

  • to have explicit mapping or explicit switch statement
  • to check toLowerCase result agains all possible keys in RoutingLocale

Copy link
Contributor

@robinandeer robinandeer left a comment

Choose a reason for hiding this comment

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

Looks good! Something to think about for the future is that we might want to redirect any URL that doesn't contain a locale but let's start like this :)

apps/store/src/lib/l10n/types.ts Outdated Show resolved Hide resolved
apps/store/src/lib/l10n/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@alebedev alebedev left a comment

Choose a reason for hiding this comment

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

Nice! Some nit comments

@@ -60,7 +60,7 @@ export const normalizeLocale = (locale: string | undefined): string | undefined
}

// We use en-SE ISO format for settings but downcase it for routing to get nicer URLs
export const routingLocale = (locale: string) => locale.toLowerCase()
export const routingLocale = (locale: LocaleValues) => locale.toLowerCase() as RoutingLocale
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we worker around with conversion function that return proper type
asRoutingLocale = (locale: LocaleValues): RoutingLocale => { return locale.toLowerCase() as RoutingLocale }

But only ways without assertion that I know are

  • to have explicit mapping or explicit switch statement
  • to check toLowerCase result agains all possible keys in RoutingLocale

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

5 participants