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

Classifier: refactor UPP data-fetching with hooks #2988

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Mar 23, 2022

Remove data-fetching from the classifier's UPP store and move it into a useProjectPreferences hook instead. Preferences should be fetched when the logged-in user changes. UPP updates are still handled by the store, so the store is updated whenever fresh preferences are received from Panoptes.

This PR builds on the user hooks in #2940, so that should be reviewed first. Following recent bugs, caused by problems with tracking user logged-in status via the UPP store, this shifts user data-fetching from our own home-grown code to SWR, which will refresh preferences in the background while the classifier is in use.

TODO:

  • check for undefined auth tokens (not null), which indicates that auth is being checked, or updated for a new user.

Package:
lib-classifier

Review Checklist

General

  • Are the tests passing locally and on Travis?
  • Is the documentation up to date?

Components

Apps

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

  • Is the changelog updated?
  • Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

@eatyourgreens eatyourgreens added experiment Something to try out and gather more information on refactor Refactoring existing code labels Mar 23, 2022
@eatyourgreens eatyourgreens force-pushed the upp-hook branch 2 times, most recently from c439429 to b654d32 Compare March 23, 2022 21:34
@eatyourgreens eatyourgreens force-pushed the private-workflows branch 4 times, most recently from cfa5390 to ccd713f Compare March 24, 2022 10:04
@eatyourgreens eatyourgreens force-pushed the upp-hook branch 2 times, most recently from f1ebe87 to f455f24 Compare March 24, 2022 10:17
@eatyourgreens eatyourgreens marked this pull request as ready for review March 24, 2022 10:44
Base automatically changed from private-workflows to master March 31, 2022 17:54
@eatyourgreens eatyourgreens force-pushed the upp-hook branch 4 times, most recently from 3ba1f61 to d4e02b2 Compare April 13, 2022 13:14
@coveralls
Copy link

coveralls commented May 10, 2022

Coverage Status

Coverage decreased (-0.03%) to 82.987% when pulling 54c78a1 on upp-hook into e3c339a on master.

@eatyourgreens eatyourgreens force-pushed the upp-hook branch 3 times, most recently from f222424 to 8b7b3c2 Compare June 11, 2022 22:56
@eatyourgreens eatyourgreens force-pushed the upp-hook branch 3 times, most recently from d3f80e6 to df9764d Compare June 14, 2022 11:45
@eatyourgreens eatyourgreens force-pushed the upp-hook branch 2 times, most recently from 23cdc0e to a685f50 Compare June 23, 2022 19:18
@eatyourgreens eatyourgreens removed the experiment Something to try out and gather more information on label Jun 29, 2022
@eatyourgreens eatyourgreens force-pushed the upp-hook branch 3 times, most recently from cc18a5a to 653c6e4 Compare July 11, 2022 05:18
@eatyourgreens eatyourgreens force-pushed the upp-hook branch 3 times, most recently from 3217b18 to 18cb2f1 Compare July 23, 2022 08:01
@mcbouslog mcbouslog self-requested a review August 18, 2022 15:25
@eatyourgreens eatyourgreens force-pushed the upp-hook branch 2 times, most recently from 5b1bda3 to 8457714 Compare August 18, 2022 16:30
@mcbouslog mcbouslog self-assigned this Aug 18, 2022
Copy link
Contributor

@mcbouslog mcbouslog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested on Planet Findes BETA with two different staging users and signed out, tutorials loaded or didn't load as expected.

Not for this PR, and more for my understanding than review notes, but I notice the hooks don't have try/catch blocks for .get/.post requests, not sure if that's because not needed or something to consider for future refactor.


import { usePanoptesAuth } from './'

const SWRoptions = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 consistent with other hooks using SWR, LGTM

}
function clear() {
self.resources.clear()
self.loadingState = asyncStates.success
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadingState is defined in the ResourceStore model

@github-actions github-actions bot added the approved This PR is approved for merging label Aug 23, 2022
Remove data-fetching from the classifier's UPP store and move it into a `useProjectPreferences` hook instead. Preferences should be fetched when the logged-in user changes. UPP updates are still handled by the store, so the store is updated whenever fresh preferences are received from Panoptes.
@eatyourgreens
Copy link
Contributor Author

One of the advantages of using the useSWR hook is that it includes try/catch for network errors. It returns both data and any error.

const { data, error } = useSWR(key, fetcher, options)

See https://swr.vercel.app/

@eatyourgreens eatyourgreens merged commit 6f00904 into master Aug 23, 2022
@eatyourgreens eatyourgreens deleted the upp-hook branch August 23, 2022 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging refactor Refactoring existing code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants