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

pageParams and queryParams dissociation #42

Open
Xample opened this issue Nov 8, 2023 · 6 comments
Open

pageParams and queryParams dissociation #42

Xample opened this issue Nov 8, 2023 · 6 comments

Comments

@Xample
Copy link

Xample commented Nov 8, 2023

Hello, while using the following code:

export const pageParams = route(':id&:date', {id: stringParser, date: dateParser}, {});

I realize that parsing the route pageParams.parseParams({id:"", date:"2001-01-01"}) the route will merge the params with the queryParams, for this reason, I'm loosing the distinction typing when I want to retrieve only one of those.

in short: I expect to have the following typing

type pageParams = {
id: string
}

type pageQueryParams = {
date?: Date
}

but currently I do get a merged version of both

type params = {
id: string;
date?: Date
}

One possible dirty (?) way of getting back only the pageParams or the pageQueryParams would be to use a Pick<params, "id"> and Omit<params, "id"> to respectively hve the pageParams and the pageQueryParams. Of course the code above should rely on another type function able to identify within the string template the possible pageQueryParams keys.

Here is an idea:

type ExcludeAmpersandPrefix<S extends string> = S extends `&:${infer _}` ? never : S;

type StringToUnion<S extends string> = 
  S extends `:${infer T}:${infer U}` ? ExcludeAmpersandPrefix<T> | StringToUnion<`:${U}`> :
  S extends `:${infer T}` ? ExcludeAmpersandPrefix<T> :
  never;

Where

type Result = StringToUnion<":id:id2&:id3">; // "id" | "id2"

But of course this is taking the problem upside down (splitting after merging) while we could have the parseParams to be a merge of pageParams and pageQueryParams.

The same apply for serializing.

Right now, we also have a problem if the pattern is "':id&:id'", i.e. a clash between 2 property names while they could be distinct.

A possible workaround is to use 2 routes, one for the pageParams ":id" and the second one for the pageQueryParams "&:date" where it will work, it's clean (single responsibilit) but then I cannot have one object to provide the route to angular i.e. I cannot use the convenient pageParams.template.

Of course I can create a helper doing something like

function mergeTemplate(a: route, b: route){
[a.template, b.template].filter((template)=>!!template).join("/");
}

But I feel like we should have the ability to parse and serialize only the pageParams and queryPageParams if we want to from typesafe-routes.

Am I doing it wrong ?

[edit] I realize that angular won't ever need the template for the pageQueryParams

@Xample
Copy link
Author

Xample commented Nov 10, 2023

Hi again, so I wrote this wrapper in the mean time:

import { AllParamNames, InferParamFromPath, Parser, route, RouteNode } from 'typesafe-routes';

type ParserMap<K extends string> = Record<K, Parser<any>>;

export type ParamsType<T extends PageParams> = ReturnType<T["parseParams"]>;
export type QueryParamsType<T extends PageParams> = ReturnType<T["parseQueryParams"]>;

export class PageParams<
    T extends string = any,
    PM extends ParserMap<AllParamNames<InferParamFromPath<T>>> = any,
    T2 extends string = any,
    PM2 extends ParserMap<AllParamNames<InferParamFromPath<T2>>> = any,
> {
    private readonly queryParams: RouteNode<T, PM, {}>;
    private readonly params: RouteNode<T2, PM2, {
        queryParams: RouteNode<T, PM, {}>
    }>;

    constructor(path: T2, pathParameters: PM2, query: T, queryParameters: PM) {
        this.queryParams = route(query, queryParameters, {});
        this.params = route(path, pathParameters, { queryParams: this.queryParams });
    }

    public getPageRoute() {
        return this.params;
    }

    public getTemplate() {
        return this.getPageRoute().template;
    }

    public parseParams(params: Parameters<typeof this.params.parseParams>[0]) {
        return this.params.parseParams(params);
    }

    public parseQueryParams(queryParams: Parameters<typeof this.queryParams.parseParams>[0]) {
        return this.queryParams.parseParams(queryParams);
    }
}

It helps explicitly dissociate route from routeParams.

The best would be to have the native support in your library, but I surrendered after watching the source code. It seems you are splitting the path into 2 categories, required and optional, with the dissociation they should be dissociated into 4 categories param-optional param-required query-optional and query-required. There is also a notion of "+" and "*" which I do not understand, I'm therefore afraid of breaking something, especially the typing which is the most powerful part of typesafe-routes :-)

@kruschid
Copy link
Owner

Hi @Xample,

The best would be to have the native support in your library

Yes, I think so as well.

splitting the path into 2 categories, required and optional

As far as I remember, the query param types were initially separated internally, but the lib never exposed that information, so that implementation detail was later removed to simplify the typing system. I need to read through the old commits again.

Thank you for the wrapper. I will look at it more closely later.

@kruschid
Copy link
Owner

kruschid commented Feb 3, 2024

Hi @Xample,

probably you don't need this anymore because it's been a while since you opened the issue. But maybe you might consider the upcoming upgrade of the lib for one of your future projects.

So I tried to separate path and search params without breaking downwards compatibility but I didn't like the first results for various reasons. I was not very happy with the current api of the library for some time and was thinking of updating it since the realease of TypeScript 4.5. However, I didn't find the time or the motivation for a major refactor until during the holidays when the typescript excitement caught me up again. ^^ You can try out the upcoming version, just npm i kruschid/typesafe-routes. A proper npm release follows soon. The complete documentation can be found here. There are also plans to include an example for angular router in the future.

Let me know if you have any feedback. Thank you. 🙏

@Xample
Copy link
Author

Xample commented Feb 5, 2024

I will check, thank you :-)
To what I see now you tried to gather all the path part directly into the routes ?! It’s a nice idea, I tried to figure out how to achieve the same but did not come with a smart solution. I’m looking forward to try the new version once ready.

The problem I actually faced with angular is that the routes are lazy loaded. I mean; the one loaded through the modules. Therefore if we have nested routes, we need to import all the nested modules unless we « possibly » import only the types of the module (skipping the implementation part)

@kruschid
Copy link
Owner

kruschid commented Feb 5, 2024

The problem I actually faced with angular is that the routes are lazy loaded. I mean; the one loaded through the modules. Therefore if we have nested routes, we need to import all the nested modules unless we « possibly » import only the types of the module (skipping the implementation part)

I'm not sure if I understand your question correctly. Maybe the following simplified example will help me understand. What speaks against defining a single route tree for your entire application, including lazy-loaded modules:

const routes = createRoutes({
  parent: {
    path: ["parent-path"]
    children: {
      child: ["child-path"],
      //... more children
    }
  },
});

...and rendering the templates in your routes:

const routes: Routes = [
  {
    path: route.template("parent"), // => "parent-path"
    loadChildren: () => import('./items/items.module').then(m => m.ItemsModule)
  }
];

as well as in your lazily-loaded routes:

const routes: Routes = [
  {
    path: route.template("parent/_child"),  // => "child-path",  the _ prefix renders the child node inlined
    component: ItemsComponent
  }
];

Would that make your route tree too large? Alternatively, you might split the route tree into multiple subtrees and lazy load them. I can see various approaches to that. One approach is to have a single main route tree that links to lazyloaded modules, which in turn contain the other route subtrees. However, deeply nested links in the main module require some redundant code. I'm not sure if that is practical. The library was not primarily designed for lazy loading. Exporting only the types would also be insufficient; as in the previous version of typesafe-routes, path rendering is dependent on individual parameter parser instances.

@Xample
Copy link
Author

Xample commented Feb 22, 2024

Hi, sorry for the delay, I was travelling with the family.
Okay I see now how it's working. Well as routes are usually not a big data there might be non need for lazy loading. What I meant is that if for instance we pack the routes aside of the angular's router (within the same file) then importing the routes will force import the module as well, and therefore it will break the lazy importation. We could possibly cheat trying to import only the type of the module (to let typescript do it's job properly) and rely on a proxy to dynamically build the routes based on the requested path, but it's a little cumbersome.
We are using your first lib's version in production, I will check if the new approach is suitable. The builder's approach was sometime hard to get it, let's see if the declarative version is better. It is also interesting to mansion that angular's path are more than just chunks of path and query params. They do have matrix parameters and route data (but we don't care the latest as it's not really part of the URL)

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

No branches or pull requests

2 participants