sandulat / routes-gen

Framework agnostic routes typings generator. Remix ☑️ SolidStart ☑️
MIT License
283 stars 12 forks source link

fix: segment parsing #43

Open lifeiscontent opened 5 months ago

lifeiscontent commented 5 months ago

the routes function had a bug I found with optional segments that aren't parameterized.

lifeiscontent commented 5 months ago

@sandulat can you please take a look at this PR?

sandulat commented 5 months ago

@lifeiscontent Hey sorry, I've missed the notification. I'll take a look tomorrow. Thanks a lot for the PR!

sandulat commented 5 months ago

@lifeiscontent can't this be solved with this simpler approach?

export function route<T extends string>(
  path: T,
  params?: Record<string, any>
): T {
  if (params) {
    const segments = path.split(/\/+/).map((segment) => {
      if (segment.startsWith(":")) {
        const keySegments = segment
          .replace(":", "")
          .replace("?", "")
          .split(/(?=\.\w+)/);

        if (keySegments[0] in params) {
          return `${params[keySegments[0]]}${keySegments.slice(1).join("")}`;
        }

        if (segment.endsWith("?")) {
          return null;
        }
      }

      return segment;
    });

    return segments.filter((value) => value !== null).join("/") as T;
  }

  return path;
}

Can you give it a try?

lifeiscontent commented 5 months ago

@sandulat re: can't this be solved with this simpler approach?

got this from test harness:

  Expected: "/products"
  Received: "/categories?/:category?/products"
lifeiscontent commented 5 months ago

@sandulat let me know if you need anything from me, the routes.d.ts file generation might need to be updated for the new param but I'm having a bit of trouble reading that code.

lifeiscontent commented 5 months ago

Hey @sandulat friendly bump on this 😅

lifeiscontent commented 5 months ago

I've simplified the algorithm to a single loop by reconstructing the path in reverse order.

lifeiscontent commented 5 months ago

@sandulat can you please take a look at this?

sandulat commented 5 months ago

Hey @lifeiscontent, I'm really sorry for the delay. I've been overloaded with work.

Regarding optional segments, I believe that instead of expanding the runtime responsibility of the route function to include them, we should instead generate all the possible types for routes with optional segments.

This means that for a route like /special?/foo, the CLI would generate these types:

This would result in the following usage:

So, in summary:

  1. The route function will NOT handle anything related to the ? symbol.
  2. The CLI will export all type variations for routes with optional segments.
  3. We can potentially apply this approach to handle scenarios like /contacts/:id.vcard (needs testing though):

    export function route<T extends string>(
    path: T,
    params?: Record<string, any>
    ): T {
    if (params) {
    return path
      .split(/\/+/)
      .map((segment) => {
        if (segment.startsWith(":")) {
          const keySegments = segment.replace(":", "").split(/(?=\.\w+)/);
    
          if (keySegments[0] in params) {
            return `${params[keySegments[0]]}${keySegments.slice(1).join("")}`;
          }
        }
    
        return segment;
      })
      .join("/") as T;
    }
    
    return path;
    }
lifeiscontent commented 5 months ago

@sandulat I think this is ready for review again, for the escaped segments, I think we can open a separate PR to fix, as the types for routes.d.ts need to be updated to include [] so we know they're escaped.