feat: remix optional segments

[lordofthecactus]

Closes: remix-run/react-router#9550
Related: remix-run/react-router#9650

As discussed this would transform remix routes/($lang)/about
into /:lang?/about which would then be matched by react-router (remix-run/react-router#9650)

/lang/about
/about

Another example /(one)/($two)/(three).($four) file routing would get transformed into `/one?/:two?/three?/:four? which then would get matched by react-router:

/
/one
/one/:two
/one/:two/three
/one/:two/three/:four

Context

Parenthesis was chosen to denote optionality since windows file systems can't have ?

Feedback needed

left some tests failing, for some edge cases. I would like feedback on how would be best to handle them or how to be more defensive about this.

• Docs
• Tests

Testing Strategy:
This test covers this code: https://github.com/remix-run/remix/compare/dev...lordofthecactus:remix:feat/remix-optional-segments?expand=1#diff-04792fa211911bb758e39db1e8bc1dfffb55657840b8f9f17fbc2330d7b346e1R41-R79

[changeset-bot]

🦋 Changeset detected

Latest commit: 85801f1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages

Name	Type
@remix-run/dev	Major
create-remix	Major
remix	Major
@remix-run/architect	Major
@remix-run/cloudflare	Major
@remix-run/cloudflare-pages	Major
@remix-run/cloudflare-workers	Major
@remix-run/deno	Major
@remix-run/eslint-config	Major
@remix-run/express	Major
@remix-run/netlify	Major
@remix-run/node	Major
@remix-run/react	Major
@remix-run/serve	Major
@remix-run/server-runtime	Major
@remix-run/vercel	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[remix-cla-bot]

Hi @lordofthecactus,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[ryanflorence]

Awesome. @brophdawg11 is 1.8.0-pre on React Router 6.4? If so let's merge this and add it to 1.9. Otherwise we'd need to wait until we're on 6.4.

EDIT: Just checked, we're not on 6.4 in dev yet. Any reason for this? Technically it should be fine?

[brophdawg11]

@ryanflorence Yep! Documenting what we discussed here. We do want to do an isolated react-router-dom 6.3.0 -> 6.4.* upgrade while still using createBrowserHistory/Router. We do not have that in 1.8.0 since that is isolated to using @remix-run/router in @remix-run/server-runtime. So I think the timeline will be:

1. Remix 1.8.0 - Uses @remix-run/router for server-side data fetching
2. Remix 1.8.1 - Upgrades to [email protected].* while still using createBrowerHistory/Router
3. React Router 6.5 - Introduces optional route segments
4. Remix 1.9.0 - Upgrades to [email protected].* + introduces conventional route support for optional route segments
   • This may also be the switch for Remix to start using createBrowserRouter/RouterProvider but will depend on timing

So I think we want to merge this after we cut the release branch for 1.8.1, which is probably early next week

[brophdawg11]

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?

[brophdawg11]

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?/*">

[ryanflorence]

["(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?

[brophdawg11]

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?">?

[ryanflorence]

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?" />

[brophdawg11]

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

[lordofthecactus]

Working now!

[brophdawg11]

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?"
]

[lordofthecactus]

Added!

[brophdawg11]

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

[lordofthecactus]

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.

[sergiodxa]

Parenthesis was chosen to denote optionality since windows file systems can't have ?

😭

[ryanflorence]

@sergiodxa don't forget, you can make your own convention in remix.config.routes and use ? if you don't have anybody using windows on your team :)

[lordofthecactus]

@ryanflorence @sergiodxa interestingly question mark is still possible on route creation 🤔

[pcattori]

@brophdawg11 so if we merge this before Remix uses RR 6.5, then optional routes won't work, correct?

So what's the rational of merging this prior to that? I get the it won't break things since we could just not tell anyone about optional routes until Remix is on RR 6.5, but then we'd have to remember to write release notes for it later. Why not just merge this after Remix uses RR 6.5?

[brophdawg11]

Yeah that would break. I'm not advocating for merging prior to that - step 4 in #4706 (comment) encompasses merging this PR and updating @remix-run/react to use RR 6.5.0 once it's released. Now that optional routes are merged in RR we can probably cut a 6.5.0-pre.0 next week over there. Then we should sync up with internally on the corresponding remix releases - we could cut a 1.8.3 on 6.4 first or we could go straight to a 1.9.0 with on 6.5.

[ryanflorence]

We can actually merge this before the router dep is updated, only the documentation needs to be delayed. IF somebody uses the syntax they'll get route paths that don't work, that's all. Which you can already do in remix config route("lol?/oops?:$%#@%").

I'd say merge it so that it's done. I'll add an issue to document it in the release where it works.

[pcattori]

We can actually merge this before the router dep is updated, only the documentation needs to be delayed. IF somebody uses the syntax they'll get route paths that don't work, that's all. Which you can already do in remix config route("lol?/oops?:$%#@%").

I'd say merge it so that it's done. I'll add an issue to document it in the release where it works.

If we do merge it now, might be best to edit the changeset to make it clear that this feature isn't ready for use yet

[pcattori]

I'm going to update the changeset to clear that^ up in a separate PR.

[kiliman]

BTW: Are optional segments a router thing? Or do I need to add support for that in the flat routes convention?

[brophdawg11]

They are a router thing but we need any route files compilation methods to compile a file-name-syntax (/my/(optional)/path in Remix) down to the router path syntax (/my/optional?/path).

[kiliman]

I see. Thanks. So as long as I generate the correct route path, I don't have to worry about expanding the optional routes to multiple routes, that's done by the router, correct?

[kiliman]

I've added optional routes support to flat routes:

<Routes>
  <Route file="root.tsx">
    <Route file="routes-flat/_public.tsx">
      <Route path="optional?" file="routes-flat/_public.(optional).tsx" />
      <Route path="about" file="routes-flat/_public.about.tsx" />
      <Route path="contact.jpg" file="routes-flat/_public.contact[.jpg].tsx" />
    </Route>
    <Route index file="routes-flat/index.tsx" />
    <Route path="test/*" file="routes-flat/test.$.tsx" />
    <Route path="users" file="routes-flat/users.tsx">
      <Route path="/:userId" file="routes-flat/users.$userId.tsx" />
      <Route path="/:userId/edit" file="routes-flat/users.$userId_.edit.tsx" />
    </Route>
  </Route>
</Routes>