Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Homepage: Build avatar and profile banner into user dashboard #6117
Homepage: Build avatar and profile banner into user dashboard #6117
Changes from 7 commits
8204ca8
087caa1
0065641
7b355e8
8ba217e
0965ff3
0685e44
6359f59
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This API request runs a search across the Users table, returning all users that match
query
. If the user ID is undefined, it will return the first 20 users from the Users table.When you want a single, specific resource by ID, the form of the URL to use is
GET /api/users/1234?include=profile_header
.You don't need an Authorization header here either, since users are public resources.
When you add Authorization headers to a request, the browser generally won't cache the response, because auth'ed requests are seen as private and not cacheable, so you might end up generating unnecessary network traffic. That can become an important consideration when you're building a busy page with a lot of users, like the Zooniverse home page.
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.
Good to know! Thanks, I'll refactor this fetch.
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.
If I'm understanding correctly I think this means we can delete auth (lines 17 and 18), remove
user_id
from thequery
, and refactor line 22 toconst { body } = await panoptes.get(
/users/${authUser.id}, query)
?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.
Yes. A unique cache key for this URL is:
With that key, the client request becomes:
Alternatively, you could set the key as the full URL:
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.
Sorry, one other thing.
panoptes.get
uses an old version of SuperAgent (I think it’s still on version 8? Version 9 came out recently, along with a couple of patches), which means that these requests aren’t usingfetch
.The app router in Next.js is built on top of
fetch
so code like this won’t be able to take advantage of Next’s built-in data-caching. Not a big deal right now, when you’re testing with just a single visitor, but something to have in mind when you roll out and need to run the home page under load.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.
How does Next.js's built-in data-caching play with SWR? This request is using SWR in a client component, which is in contrast to using
fetch
directly in a page.js in app-root:front-end-monorepo/packages/app-root/src/app/page.js
Lines 22 to 24 in 9a7a630
(Linking the existing superagent Issue to this comment: #317)
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.
In the app router, with Next.js 13.5 and higher, Vercel replace native
fetch
with their own globalfetch
that caches the return value.https://nextjs.org/docs/app/building-your-application/data-fetching/fetching-caching-and-revalidating#fetching-data-on-the-server-with-fetch
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.
SWR can be used with
fetch
, likereact-query
, but will work with any function that returns a Promise-like object.This page has notes on using it with the app router.
https://swr.vercel.app/docs/with-nextjs
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.
Here’s some documentation on data caching with libraries that don’t use
fetch
, such aspanoptes-js
.https://nextjs.org/docs/app/building-your-application/data-fetching/fetching-caching-and-revalidating#fetching-data-on-the-server-with-third-party-libraries
Here’s documentation for the app router data cache, which you’ll need to manage across app instances and deployments, since Zooniverse Next.js is self-hosted on Node.
https://nextjs.org/docs/app/building-your-application/caching#data-cache
I haven’t played around with app router caching, but I believe you can also cache rendered HTML across requests, similar to how the current pages router setup serves pages via a CDN, so that HTML can be served immediately, without waiting for a page build to run.
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.
Here’s a couple of old issues and PRs about slow page build times, for context. Worth bearing in mind for both the new home page and the user stats pages.
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.
If
authUser
is empty or null, you should set the key to null. That tells SWR to skip the request.Otherwise, your code here might inadvertently run the SWR hook when it shouldn't, and cache an unexpected value for this key in the SWR cache.
When you pass a key to
useSWR
, it looks up that key in its global cache and immediately returns the stored result (which is why keys must be unique.) If the stored result is stale, it revalidates the cache by fetching a new result from the remote source. Hence 'stale while revalidate.'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.
Also a warning that if you've used
{ authUser }
as the key for any other queries, elsewhere in your code,useSWR(key, fetcher, options)
will returned the cached values for those queries. That can lead to some weird bugs.You could avoid that here by passing the
query
object as the key. I think that's unique to this specific API request. I think that when I was writing SWR hooks for translations, I used the API endpoint and query as the key for each request, but I don't remember the details. That was where I learnt that reusing a key leads to bugs!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.
DashboardContainer is in UserHome, and UserHome only renders if
authUser.login
exists. I don't expectauthUser
to ever be null in this component.UserHome is conditionally rendered in app-root here. Do you suggest handling the homepage differently?
front-end-monorepo/packages/app-root/src/components/HomePageContainer.js
Line 24 in 4deea7e
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.
If you know that
authuser
will always be defined, then this should be fine.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.
Sounds good. Good catch with the need for a unique
key
too, so I'll definitely address that here!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.
I think that unique key caught me out here, which is why I added the endpoint.
front-end-monorepo/packages/lib-react-components/src/hooks/usePanoptesUser.js
Lines 32 to 35 in 4deea7e
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.
Not related to this specific API request, but it can also be a good idea to invalidate the cache when an auth token is refreshed, like this example for user project preferences, which includes
authorization
in the key:front-end-monorepo/packages/lib-classifier/src/hooks/useProjectPreferences.js
Line 51 in 4deea7e
I think there might be some discussion of that in the SWR docs for query keys. The idea is that you don't want to accidentally show stale private data to another user, eg. on a shared browser, so always clear the cache when the auth header expires and is refreshed.
https://swr.vercel.app/docs/arguments#multiple-arguments