zilch / type-route

The flexible, type safe routing library.
https://type-route.zilch.dev
MIT License
424 stars 15 forks source link

Query Parameter value types. #106

Closed seivan closed 2 years ago

seivan commented 2 years ago

So these two, almost identical, with the exception that the former ( guessing default serializer behaviour) quotes around the value.

    category: param.query.ofType<"first" | "second">()
    category: param.query.ofType({
      parse(raw) {
        if (raw === "second") {
          return raw
        }
        else {
          return "first"
        }
        // return noMatch
      },
      stringify(value): "first" | "second" {
        return value === "second" ? value : "first"
      }
    })

The latter does it without the quote marks around the values category=first while the former does category="first". Obviously the former is far more convenient to write and gives you a strong requirement on passing params as they are required, and they are required as specific values.

Wish there was a way to narrow the types for most of the values without diving into custom serializerers.

bradenhs commented 2 years ago

Yeah the default serializer just uses JSON.stringify / JSON.parse so you end up with quotes around simple values like strings. I'm not sure the best way to make this process more simple but really appreciate you bringing this up! I'll give this some thought.

seivan commented 2 years ago

Yeah the default serializer just uses JSON.stringify / JSON.parse so you end up with quotes around simple values like strings. I'm not sure the best way to make this process more simple but really appreciate you bringing this up! I'll give this some thought. Love this library and the thoughts behind it, it's such a relief that it exists, so I like to see it improve.

Have you considered a different API for defining params that are more idiomatic when it comes to defining types?

Would something likenew Params<type>() not work?

new Params<type>()

new Params<"first" | "second">({default: "first})

new Params<"first" | "second">({optional:true})

new Params<Array<number | string>({default: [0, "hello"]})

Edit: The values used as arguments can be types of their own, that way you can use them in the extends for route-definition.

As I understand it, you want to narrow down the types, which is why the current api is params.key That way you can use type guards for the params, in both defining the route as well as pushing routes.

Since param.query.string exists, I am guessing that whatever generic constraints are used on that to use string as statically typed on route call site, could be used with the proposed api. Whatever logic it uses to do the parsing for string can be reused for T extends string The kicker is, it doesn't really matter what it does internally for strings, the purpose is to get type safety at call site of pushing new routes with params.

Am I off track here?

seivan commented 2 years ago

Yeah the default serializer just uses JSON.stringify / JSON.parse so you end up with quotes around simple values like strings.

Right, the other option to what I mentioned above, is somehow letting param.query.string define allowed strings. Maybe that's more digestible?

bradenhs commented 2 years ago

This serializer factory may be helpful if you have a lot of scenarios where you want to specify a finite list of strings as acceptable values for a route parameter:

function createStringSerializer<T extends string>(
  allowedValues: T[]
): ValueSerializer<T> {
  return {
    parse: (raw: T) => (allowedValues.includes(raw) ? raw : noMatch),
    stringify: (value) => value,
  };
}

defineRoute(
  {
    foo: param.query.ofType(createStringSerializer(["first", "second"])),
  },
  () => "/bar"
);
seivan commented 2 years ago

Thanks, I've done something similar right now as a work around, but I do believe there is room for defining type safety even if parsing is involved at runtime.

But... I dunno, maybe you're on to something, since this has to go both ways? Maybe just solving compile time checking values isn't sufficient, since it has to be parsed from strings anyway and it would just hide the errors in case a noMatch was passed in?

string should technically accept it, even if compile time checks expects a "certain" string, so maybe serializer is the only option here.

If that's the case, I actually suggest adding that helper in a little "useful" helper export, same place defaultClick, and attemptScrollup is and close this ticket.

I think I got it all wrong, heh.

bradenhs commented 2 years ago

Yeah Type Route does need to have an explicit parser for values to ensure values of incorrect type don't sneak there way into the application. Technically the param.query.ofType<"first" | "second">() construct is unsafe. Consider this URL: /path?myParam=1. That wouldn't result in an error when parsed b/c the parser itself doesn't know 1 isn't a valid type. Then you run into scenarios where you think the value you're dealing with is "first" | "second" when in reality it is 1 which likely results in runtime errors or unexpected behavior. Now you'd be fine if your application has 100% control of the URL since you know you're putting correct values there but the reality is your application does not have 100% control of the URL since the user can input whatever they want there.

bradenhs commented 2 years ago

If that's the case, I actually suggest adding that helper in a little "useful" helper export, same place defaultClick, and attemptScrollup is and close this ticket.

This is a great idea! I think I'll go ahead and add something like this.

bradenhs commented 2 years ago

For additional safety with really complex route parameters I use this simple integration with https://github.com/pelotom/runtypes in some applications I work on. Since using ofType<T>() without an explicit serializer isn't safe I never use it personally. I'm not sure if the following serializer is worth including in Type Route itself but it might be a good addition to the docs.

import * as type from "runtypes";

function createRuntypeValueSerializer<T>(
  runtype: type.Runtype<T>
): ValueSerializer<T> {
  return {
    parse: (raw) => {
      try {
        return runtype.check(JSON.parse(raw));
      } catch {
        return noMatch;
      }
    },
    stringify: (value) => JSON.stringify(value),
  };
}

const fooBarSerializer = createRuntypeValueSerializer(
  type.Record({
    foo: type.String,
    bar: type.Number
  })
);

defineRoute(
  {
    fooBar: param.query.ofType(fooBarSerializer)
  },
  () => "/baz"
);
seivan commented 2 years ago

Oh wow, that's an amazing idea. I am using https://github.com/brielov/typed#readme but I guess the concept is the same.

without an explicit serializer isn't safe I never use it personally

Yeah this is probably a sound rule, thanks! .

Feel free to close this ticket, or base it on doc updates. Your call. I'm good!

Again, thank you!