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

alt approach to knowing what params are optional #464

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stackoverfloweth
Copy link
Contributor

The Background

Currently, anything that can have params is normalized to WithParams type. This type has a params property that stores Record<string, Param>. The key for each param is where we store if we expect the param to be optional or not.

For example:

{ value: '/[foo], params: { "foo": Boolean } }

This WithParams has a single param "foo", which we can see is NOT optional.

{ value: '/[?foo], params: { "?foo": Boolean } }

Versus this WithParams, where "foo" is now optional.

The Motivation

We've never loved sticking the leading "?" in the params object for several reasons

  • it's ugly
  • it's changing the keys as defined by the user
  • when we check for duplicate param names we need to look for the same name in twice where one has the question mark
  • we combine two withParams objects we have to de-normalize param names back to exclude the question mark
  • it creates a situation where there are 2 possibly conflicting sources of truth, the user already provides the optional declaration in the value property

That last reason is one that I found the most compelling and why I chose to try this syntax. This is also something that I didn't love about the suggestion to introduce a new optional property that only tracks what params are optional.

The Solution

Now WithParams leaves params alone and looks at the value to determine what's optional.

{ value: '/[foo], params: { "foo": Boolean } } // required
{ value: '/[?foo], params: { "foo": Boolean } } // optional

What About The Types?

I fully implemented this change at runtime, got tests passing, tested with router-preview, and totally expected types to be broken. Much to my surprise it seems the types already work in the way the runtime now works?

@stackoverfloweth stackoverfloweth self-assigned this Feb 8, 2025
Copy link

netlify bot commented Feb 8, 2025

Deploy Preview for kitbag-router ready!

Name Link
🔨 Latest commit 631a6fa
🔍 Latest deploy log https://app.netlify.com/sites/kitbag-router/deploys/67a7dbd99f25450008834494
😎 Deploy Preview https://deploy-preview-464--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.

Copy link
Contributor

@pleek91 pleek91 left a comment

Choose a reason for hiding this comment

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

Nice, I like this direction 👍

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