-
Notifications
You must be signed in to change notification settings - Fork 53
Use dynamic routes for /community pages #1285
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
base: master
Are you sure you want to change the base?
Conversation
ec579d4 to
40e4939
Compare
In practice, not marking it as such has worked fine because the only
usage is with a parent that is a Client Component, meaning it is
implicitly used as one as well.
Not marking it is fragile. Without "use client", using it in a parent
that is a Server Component (done in the next commit) results in a run
time error:
You're importing a component that needs useEffect. It only works in a
Client Component but none of its parents are marked with "use client",
so they're Server Components by default.
This reorganizes files from a flat structure with everything under a catch-all [[...community]] to a nested structure with one directory per URL part. Note that "narratives" is an optional part in the path. Such logic was easily implemented in the custom router by using Array.shift(), but it can't be encoded into dynamic routes. As a result, there is some boilerplate duplication between community/[user]/ and narratives/[user]/, but the bulk of the content remains in the top-level community/*.tsx files.
The new file structure makes it clear that this component is only used when there is no repo in the URL path.
44da889 to
e7abe5a
Compare
| * Note that the 404 status codes are being set/returned at the | ||
| * Express router code, as a side effect of how that code re-directs | ||
| * requests that don't directly load a dataset or narrative into the | ||
| * Next.js part of the web site. This router code does not make use of | ||
| * the Next.js `notFound()` method because the 404 status has already | ||
| * been set. For the requests that don't display an <ErrorMessage>, | ||
| * the status should _arguably_ be 200, but I'm not going to worry | ||
| * about that at the moment. - @genehack, 1 Aug 2025 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note on previous implementation: this isn't entirely true. 404 is only returned for some routes, and never for text/html. Examples:
curl -I "https://nextstrain.org/community/invalid"
# 200
curl -I "https://nextstrain.org/community/user/invalid"
# 404
curl -I "https://nextstrain.org/community/user/invalid" -H 'accept: text/html'
# 200
curl -I "https://nextstrain.org/community/user/repo/invalid"
# 404
curl -I "https://nextstrain.org/community/user/repo/invalid" -H 'accept: text/html'
# 200This contrasts with /staging which returns 404 for text/html:
curl -I "https://nextstrain.org/staging/invalid" -H 'accept: text/html'
# 404There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered adding another commit to send proper 404 responses, primarily to address the related testing comment. That was fairly straightforward for https://nextstrain.org/community/user/repo/invalid: 259d795
However, it's complicated for https://nextstrain.org/community/user/invalid which relies on a client-side fetch to /charon to determine whether the repo exists and is public. The fetch could be moved server-side, but I didn't want to deal with that in this PR.
Description of proposed changes
This PR has (in order) 1 cleanup/prep commit, 1 main commit, and 1 follow-up commit.
The main commit reorganizes files from a flat structure with everything under a catch-all [[...community]] to a nested structure with one directory per URL part.
Note that "narratives" is an optional part in the path. Such logic was easily implemented in the custom router by using Array.shift(), but it can't be encoded into dynamic routes. As a result, there is some boilerplate duplication between community/[user]/ and narratives/[user]/, but the bulk of the content remains in the top-level community/*.tsx files.
Related issue(s)
Follow-up to #1274
Checklist