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

fix: segment parsing #43

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

Conversation

lifeiscontent
Copy link

@lifeiscontent lifeiscontent commented Mar 30, 2024

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

@lifeiscontent
Copy link
Author

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

@sandulat
Copy link
Owner

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

@sandulat
Copy link
Owner

sandulat commented Mar 31, 2024

@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
Copy link
Author

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

got this from test harness:

  Expected: "/products"
  Received: "/categories?/:category?/products"

@lifeiscontent
Copy link
Author

@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
Copy link
Author

Hey @sandulat friendly bump on this 😅

@lifeiscontent
Copy link
Author

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

@lifeiscontent
Copy link
Author

@sandulat can you please take a look at this?

@sandulat
Copy link
Owner

sandulat commented Apr 6, 2024

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:

  • /foo
  • /special/foo

This would result in the following usage:

  • route('/foo')
  • route('/special/foo')

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
Copy link
Author

@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.

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