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

feat: remix optional segments #4706

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions .changeset/moody-pants-own.md
@@ -0,0 +1,25 @@
---
"@remix-run/dev": minor
---

feat: remix optional segments

Allows for the creation of optional route segments by using parenthesis. For example:
Creating the following file routes in remix `/($lang)/about`
this will match the following routes
```
/en/about
/fr/about
/about
```

helpful for optional language paths.

Another example `/(one)/($two)/(three).($four)` file routing would match
```
/
/one
/one/param1
/one/param1/three
/one/param1/three/param2
```
1 change: 1 addition & 0 deletions contributors.yml
Expand Up @@ -253,6 +253,7 @@
- lili21
- lionotm
- liranm
- lordofthecactus
- lpsinger
- lswest
- lucasdibz
Expand Down
41 changes: 41 additions & 0 deletions packages/remix-dev/__tests__/routesConvention-test.ts
Expand Up @@ -36,6 +36,47 @@ describe("createRoutePath", () => {
["[index]", "index"],
["test/inde[x]", "test/index"],
["[i]ndex/[[].[[]]", "index/[/[]"],

// Optional segment routes
["(routes)/$", "routes?/*"],
["(routes)/($)", "routes?/*?"], // TODO: Fails, do we want to allow this?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to support optional splats as they're already optional. path="child/*" will match both /child and /child/any/other/path: See https://codesandbox.io/s/optional-splat-segments-xpn3kq

Maybe we throw an error on ($) syntax?

["(routes)/(sub)/$", "routes?/sub?/*"],
["(routes).(sub)/$", "routes?/sub?/*"],
["(routes.sub)/$", "routes?/sub?/*"], // TODO: Fails, do we want to allow this?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I don't think we need to add the complexity of being able to interpolate dots inside an optional segment - the line above is how I'd expect optional segments + dot-notation to work together. In this case I'd expect the generated React Router path to be <Route path="routes.sub?/*">

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

["(routes.sub)/$", "routes?/sub?/*"] // do we want to allow this?

The word "routes" there is confusing to me, so I'm changing it to "products.all" for my comment 😅

["(products.all)/$", "products?/all?/*"] // do we want to allow this?

No, we don't. The React Router version won't have a way of "grouping" two segments as optional together either:

// this is only for "sub"
<Route path="products/all?" />

// this is for both
<Route path="products?/all?" />

So in Remix it should be the same, only individual segments can be optional:

(products).(all).tsx -> products?/all?

["(routes)/($slug)", "routes?/:slug?"],
["(routes)/sub/($slug)", "routes?/sub/:slug?"],
["(routes).sub/($slug)", "routes?/sub/:slug?"],
["($)", "*?"], // TODO: Fails, do we want to allow this?
["(nested)/$", "nested?/*"],
["(flat).$", "flat?/*"],
["($slug)", ":slug?"],
["(nested)/($slug)", "nested?/:slug?"],
["(flat).($slug)", "flat?/:slug?"],
["flat.(sub)", "flat/sub?"],
["(nested)/(index)", "nested?"], // Fails with `"flat?/index?"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to do an "optional" index route in react router since ? is only for use with path. Optional segments are for when you want to use the same element for multiple <Route> paths, but that wouldn't make sense for index routes:

// You wouldn't need to do this since `/nested` already renders <Nested /> and 
// therefore doesn't need the index route to add it
<Route path="nested" element={<Nested />}>
  <Route index element={<Nested />} />
  <Route path="child" element={<Child />} />
</Parent/>

// So you would just do and the `Outlet` inside of `<Nested/>` would be a no-op 
// when you were at `/nested`
<Route path="nested" element={<Nested />}>
  <Route path="child" element={<Child />} />
</Parent/>

I haven't ever tried this but I assume if you want an actual /index URL you would escape it like routes/nested/[index].tsx? Lets' test that - and if that's the case, would we need to support /routes/nested/([index]).tsx to output <Route path="nested/index?">?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, [] are the escape characters to escape any of our file conventions. Should make sure it works with (), also.

// match literal paren characters in the URL
routes/[(foo)].tsx => <Route path="(foo)" />

// match literal `index` in an optional segment
routes/funds/([index]).tsx => <Route path="index?" />

["(flat).(index)", "flat?"], // Fails with `"flat?/index?"`
["(index)", undefined], // Fails with `"index?"`"
["(__layout)/(index)", undefined], // Fails with __layout?/index?
["__layout/(test)", "test?"],
["__layout.(test)", "test?"],
["__layout/($slug)", ":slug?"],
["(nested)/__layout/($slug)", "nested?/:slug?"],
["($slug[.]json)", ":slug.json?"],
["(sub)/([sitemap.xml])", "sub?/sitemap.xml?"],
["(sub)/[(sitemap.xml)]", "sub?/sitemap.xml?"], // TODO should this have been sub?/(sitemap.xml)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think this looks like a bug since the [] escaping isn't working right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working now!

["(posts)/($slug)/([image.jpg])", "posts?/:slug?/image.jpg?"],
[
"($[$dollabills]).([.]lol)[/](what)/([$]).($)", // TODO outputs ":$dollabills?/.lol?/what?/$?/:?"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is an escaped [/] allowed? That seems odd to me. And then we don't need to support ($) so I think this test could become something like:

[
  "($[$dollabills]).([.]lol)/(what)/([$]).($up)", 
  ":$dollabills?/.lol?/what?/$?/:up?"
]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

":$dollabills/.lol/what/$/*",
],
["(sub).([[])", "sub?/[?"],
["(sub).(])", "sub?/)?"],
["(sub).([([])])", "sub/[]"], // Fails with "sub?/([)]?"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯 uhhh - I guess the intention here is sub?/([])??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems from before there is not a clear handling of multiple brackets within brackets. Either you close on the first match or get the longest within multiple separators? feels like this should be a different PR and discussion.

["(sub).([[])", "sub?/[?"],
["(beef])", "beef]?"],
["([index])", "index?"],
["(test)/(inde[x])", "test?/index?"],
["([i]ndex)/([[]).([[]])", "index?/[?/[]?"],
];

for (let [input, expected] of tests) {
Expand Down
15 changes: 14 additions & 1 deletion packages/remix-dev/config/routesConvention.ts
Expand Up @@ -190,7 +190,20 @@ export function createRoutePath(partialRouteId: string): string | undefined {
result = result.replace(/\/?index$/, "");
}

return result || undefined;
return generateOptionalDynamicRoutes(result) || undefined;
}

function generateOptionalDynamicRoutes(routeId: string) {
let segments = routeId.split("/");

let newSegments = [];
for (let segment of segments) {
console.log(segment, "segment");
console.log(segment.replace(/^\((.+)\)$/, "$1"));
newSegments.push(segment.replace(/^\((.+)\)$/, "$1?"));
}

return newSegments.join("/");
}

function findParentRouteId(
Expand Down