remix icon indicating copy to clipboard operation
remix copied to clipboard

[Bug]: Route filename starting with escaped at-sign [@] does not match

Open soungrong opened this issue 3 years ago • 10 comments

Which Remix packages are impacted?

  • remix (Remix core)
  • @remix-run/react

What version of Remix are you using?

1.0.6

Steps to Reproduce

  1. Create a file [@]hello.jsx in app/routes
  2. Visit https://example.com/@hello

Expected Behavior

The route matches successfully.

Actual Behavior

No routes matched location "/@hello"
GET /@hello 404 - - 17.564 ms

soungrong avatar Dec 02 '21 06:12 soungrong

Related: https://github.com/remix-run/remix/issues/819

@kentcdodds tried adding (["[@]hello", "@hello"]) to the test cases https://github.com/remix-run/remix/blob/9fc3dfb8b2234921c05d115ad52ebd7e812e866b/packages/remix-dev/tests/routesConvention-test.ts#L38 which weirdly passes.

soungrong avatar Dec 02 '21 07:12 soungrong

From what I can tell, the route objects and pathname look correct—but matchRoutes is returning a falsey value here: https://github.com/remix-run/remix/blob/a69a631cb5add72d5fb24211ab2a0be367b6f2fd/packages/remix-server-runtime/routeMatching.ts#L16

soungrong avatar Dec 03 '21 04:12 soungrong

@soungrong see #819. I have a patch that works. Current implementation doesn't match paths that start with non-word character like @ or .

kiliman avatar Dec 08 '21 00:12 kiliman

@kiliman LGTM, thank you!

soungrong avatar Dec 08 '21 01:12 soungrong

The patch suggested in https://github.com/remix-run/remix/issues/819#issuecomment-988337149 actually fails one of the tests here https://github.com/remix-run/react-router/blob/main/packages/react-router/tests/matchPath-test.tsx#L156-L162

Here's what I've learned so far trying to debug this with my primitive understanding of react-router's internals. Hopefully it helps to move this issue forward:

  1. This loop iterates over all the available route objects and the branch[es] they contain—attempting to match the requested path e.g. /@hello with an existing branch's path. https://github.com/remix-run/react-router/blob/8f63699bac4006294619a4c1fb317aba515b09b7/packages/react-router/index.tsx#L827-L829

  2. It keeps iterating deeper (a.k.a. nested paths) into the route object as long as it keeps finding successive matches. https://github.com/remix-run/react-router/blob/8f63699bac4006294619a4c1fb317aba515b09b7/packages/react-router/index.tsx#L969

  3. The actual matching is done inside matchPath, with RegEx. https://github.com/remix-run/react-router/blob/8f63699bac4006294619a4c1fb317aba515b09b7/packages/react-router/index.tsx#L976-L979

  4. In order for matchPath to find a match, it uses compilePath to first build a suitable RegEx. https://github.com/remix-run/react-router/blob/8f63699bac4006294619a4c1fb317aba515b09b7/packages/react-router/index.tsx#L1095-L1099

  5. However no match is found for /@hello because the regex used /^\/(?:\b|$)/i in the first iteration of the loop from [2.] does not match anything. It should match for the root path / a.k.a. just the leading slash of the requested path in this first iteration.

  6. In the second iteration, it should be a full exact match with /@hello using something like ^\/@hello\/*$/i and return the route object to end the loop. (but I couldn't solve this part)

That said, nested paths without leading special characters currently match as expected, e.g. /hey/@hello

soungrong avatar Dec 10 '21 12:12 soungrong

Yes, the patch does break the test /user2 matches /user. I'm actually trying to come up with the correct solution, but everything I've tried so far seems to break other tests.

I'm still learning how the pattern matching process works.

kiliman avatar Dec 10 '21 14:12 kiliman

The underlying issue seems to have been fixed in react-router : https://github.com/remix-run/react-router/pull/8877

machour avatar May 20 '22 15:05 machour

I believe that this will be released in react-router 6.4.0. Until then I have been using a Makefile patch:

patch-react-router:
	# https://github.com/remix-run/react-router/pull/8877/files#diff-cf8c561ed8c0d6c0ace3e6be06dcceaa55f84f916d19f6d6247c208774982921R480
	sed -i.bak "s/\[\.\~\-\]/\[\@\.\~\-\]/g" ./node_modules/react-router/main.js
	sed -i.bak "s/\[\.\~\-\]/\[\@\.\~\-\]/g" ./node_modules/react-router/index.js
	sed -i.bak "s/\[\.\~\-\]/\[\@\.\~\-\]/g" ./node_modules/react-router/react-router.development.js
	sed -i.bak "s/\[\.\~\-\]/\[\@\.\~\-\]/g" ./node_modules/react-router/react-router.production.min.js
	sed -i.bak "s/\[\.\~\-\]/\[\@\.\~\-\]/g" ./node_modules/react-router/umd/react-router.development.js
	sed -i.bak "s/\[\.\~\-\]/\[\@\.\~\-\]/g" ./node_modules/react-router/umd/react-router.production.min.js

Probably much better ways of doing this, but this seems to work until the upgrade is released.

rowanc1 avatar Jun 30 '22 03:06 rowanc1

I had the same problem when using remix 1.7.2 This problem only occurs when trying to match root route.

For example

/xxx/@user can be match with app/routes/xxx/$user.tsx

But /@user can not be match with app/routes/$user.tsx

St2r avatar Oct 03 '22 16:10 St2r

I created a patch to backport React Router PR #9300 to React Router 6.3 which is the version pinned in Remix.

This fix allows non-alphanumeric characters to start a route path. This means /@user or /.well-known/ or even /😍 will work.

Currently this only patches the development build as the production files are minified. I will update the patch once I setup to build React Router locally.

https://gist.github.com/kiliman/1a8eb57a6558c96d292bb913add5a178

kiliman avatar Oct 13 '22 15:10 kiliman

Any news?

AnushavanGhulyan-melon avatar Dec 16 '22 16:12 AnushavanGhulyan-melon

I upgraded to the latest version of remix (v1.10.0-pre.5) which comes with react-router v6.6.1 - but it is still not possible to achieve this style of user profile URLs (a la Mastodon or TikTok)

I've also tried defining the route manually in remix.config.js and this also does not work. There are some comments in the react-router repo that suggest this used to work, but has regressed since v6.5

Are there no other workarounds?

mattfysh avatar Dec 26 '22 00:12 mattfysh

Is it possible to use parameterized pattern-matching with remix file system similarly to react-router routes ?

<Route path="/@:userId" element={<UserElement />}/>

kael avatar Dec 26 '22 08:12 kael

Not a permanent solution but it seems that with the latest (stable) version it is possible to handle this in $handle.tsx by parsing the params.handle:

export async function loader({ request, params }: LoaderArgs) {
  let handle = params.handle.startsWith("@") ? params.handle.slice(1) : null;

  if (!handle) {
     throw new Response("Not Found", {
      status: 404,
    });
  }

  // ...
}

giuseppeg avatar Dec 29 '22 21:12 giuseppeg

it is possible to handle this in $.tsx by parsing the url pathname

Yes, that's what I finally did. Not as elegant as react-router way, and I might need to handle some more cases, but it works.

kael avatar Dec 30 '22 09:12 kael

Just to summarize this issue before I close it:

  1. As far as the issue description is concerned, this bug is fixed as of remix v1.10.0 and you can URL match /@hello as long as you're using a static pattern, e.g. filename [@]hello.tsx.

  2. If you want to URL match /@user or /@user2 or /@user3 dynamically with a single route, then that entire segment must be dynamic, and then handled/routed by yourself. Like this sample https://github.com/remix-run/remix/issues/846#issuecomment-1367587376

This was confirmed by @brophdawg11 in RR 6.5.0 https://github.com/remix-run/react-router/releases/tag/react-router%406.5.0 and PR here https://github.com/remix-run/react-router/pull/9506

  1. If for some reason you still want to use something like filename [@]$username.tsx and pin yourself to remix v1.8.2 then that's actually possible with the patch here: https://github.com/remix-run/remix/issues/846#issuecomment-1277776319

soungrong avatar Jan 11 '23 06:01 soungrong

Any updates on this? This is still broken in v1.12. The workarounds listed by @soungrong are useful but this still needs to be fixed. Having a set of urls start with a prefix followed by a dynamic segment is a pretty common occurrence.

koderyoda avatar Feb 11 '23 03:02 koderyoda

If for some reason you still want to use something like filename [@]$username.tsx

The reason is that we want the router to perform all routing requirements, and not have to create a wide dynamic segment then manually filter out values that do not begin with the intended prefix manually.

What I mean by this is having to perform something like this in every nested route:

invariant(params.username && params.username[0] === '@' && params.username.length > 1, 'invalid')

When the desired routing is /[@]$username but we are left only with the option to use /$username then any incoming request where the first character is not '@' requires custom 404 handling

The router should be the perfect place for this behaviour to be implemented

mattfysh avatar Feb 11 '23 04:02 mattfysh