fenok / react-router-typesafe-routes

Comprehensive and extensible type-safe routes for React Router v6 with first-class support for nested routes and param validation.
MIT License
145 stars 3 forks source link

v6 Direction for path param parsing #10

Closed thejuan closed 1 year ago

thejuan commented 2 years ago

We started playing with this package for v6 (which is looking great 👍 ) when it was using parsers like numberParser these threw if it was bad input. Now with the move to numberType you either pass a fallback or the returned value could be undefined which requires checking for bad input in every route or (just swallowing it by passing -1 or NaN)

Whilst the errors were jarring at first, using an ErrorBoundary you could handle them globally or per nested set of routes, especially if the error thrown was rich.

Curious on what you are thinking for handling parse errors going forward (just rely on fallbacks, is there an option to still throw)?

thejuan commented 2 years ago

There also doesn't seem to be a way to specify fallbacks for oneOf?

fenok commented 2 years ago

There also doesn't seem to be a way to specify fallbacks for oneOf?

Something like this should work:

oneOfType(...Object.values(Direction))(Direction.Ascending)

Granted, it may be a bit confusing.oneOfType is not a type, but a function returning a type.

Regarding throwing vs returning undefined. Throwing was rather a legacy from the previous implementation than a thoughtful decision. In v5, there were regexps for path params, and parsers were used to properly type them. React-router v5 wouldn't match paths with invalid params, and therefore throwing was perfectly fine, because it meant that there is a developer error and the parser in question doesn't match the regexp. (Or that the component is being rendered on an unexpected path.)

In contrast, react-router v6 will happily match routes with any parameters. In other words, we now have to deal with invalid parameters in our components. I think that returning undefined enables more fine-grained control in this situation. It's also similar to what react-router docs propose, and it also aligns with how types (like numberType) work for search params and state.

Note that implicit params will never be undefined because there is no parsing involved.

For now, if you need to throw errors, you can do it like this:

// Define a helper function
function assertIsDefined<T>(value: T | null | undefined): asserts value is T {
  if (value === null || value === undefined) {
    throw new Error('Assertion failed: Expected value to be defined.');
  }
}

// In some component
const { id } = useTypedParams(ROUTES.MY_ROUTE);
assertIsDefined(id); // After this line TS knows that id is not undefined (or null)

While I'm certain that returning undefined should be enabled by default, I understand that some users might want to always throw errors instead. I'll think about providing a way to configure it.

fenok commented 2 years ago

It's worth noting that I updated the library to return undefined fields instead of omitting them, so now it's possible to assert the whole params object:

function required<T extends Record<string, unknown>>(obj: T): Required<T> {
    if (Object.values(obj).includes(undefined)) throw new Error("Unexpected undefined");

    return obj as Required<T>;
}

// In some component
const { id } = required(useTypedParams(MY_ROUTE)); // TS knows that id is not undefined

Also, version 0.4.0 is now released 🥳.

The above-mentioned option is delayed for some future release, because I'm still not sure how to properly implement it 😅

thejuan commented 2 years ago

Thanks, I'm in the process of upgrading now. It really feels like the types should have the optionality baked in. i.e. requiredNumberType or numberType({required:true}) Or even allow fallback to be a function which could throw and allow us to make a requiredNumberType

fenok commented 2 years ago

It really feels like the types should have the optionality baked in.

Yeah, that's what I've eventually settled with. In v0.4.3, it's now possible to specify a special throwable fallback like this:

import { throwable } from "react-router-typesafe-routes/dom"; // Or /native

const ROUTE = route("route/:id", { params: { id: numberType(throwable) } });

This way, in case of a parsing error, the original error will be thrown, and TS is aware of that.

fenok commented 1 year ago

Closing as this seems to be resolved by #16. Please reopen if I'm missing something.

thejuan commented 1 year ago

Amazing, thanks!