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

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

Closed
soungrong opened this issue Dec 2, 2021 · 18 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@soungrong
Copy link

soungrong commented Dec 2, 2021

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 soungrong added the bug Something isn't working label Dec 2, 2021
@soungrong
Copy link
Author

Related: #819

@kentcdodds tried adding (["[@]hello", "@hello"]) to the test cases

["[i]ndex/[[].[[]]", "index/[/[]"]
which weirdly passes.

@soungrong
Copy link
Author

From what I can tell, the route objects and pathname look correct—but matchRoutes is returning a falsey value here:

let matches = matchRoutes(routes as unknown as RouteObject[], pathname);

@kentcdodds kentcdodds assigned kentcdodds and jacob-ebey and unassigned kentcdodds Dec 3, 2021
@kiliman
Copy link
Collaborator

kiliman commented Dec 8, 2021

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

@soungrong
Copy link
Author

@kiliman LGTM, thank you!

@soungrong
Copy link
Author

The patch suggested in #819 (comment) 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

@kiliman
Copy link
Collaborator

kiliman commented Dec 10, 2021

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.

@machour
Copy link
Collaborator

machour commented May 20, 2022

The underlying issue seems to have been fixed in react-router : remix-run/react-router#8877

@rowanc1
Copy link

rowanc1 commented Jun 30, 2022

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.

@St2r
Copy link

St2r commented Oct 3, 2022

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

@kiliman
Copy link
Collaborator

kiliman commented Oct 13, 2022

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

@AnushavanGhulyan-melon
Copy link

Any news?

@mattfysh
Copy link

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?

@kael
Copy link

kael commented Dec 26, 2022

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

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

@giuseppeg
Copy link

giuseppeg commented Dec 29, 2022

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,
    });
  }

  // ...
}

@kael
Copy link

kael commented Dec 30, 2022

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.

@soungrong
Copy link
Author

soungrong commented Jan 11, 2023

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 [Bug]: Route filename starting with escaped at-sign [@] does not match #846 (comment)

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 remix-run/react-router#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: [Bug]: Route filename starting with escaped at-sign [@] does not match #846 (comment)

@koderyoda
Copy link

koderyoda commented Feb 11, 2023

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.

@mattfysh
Copy link

mattfysh commented Feb 11, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests