Support fetching gist with token when signed in #936
Conversation
There was a problem hiding this comment.
Looks good! Left a few nitpicks, but I wouldn't consider those blocking for this PR, just some nice code cleanup that could help if you have spare time 😁
I didn't see a CONTRIBUTING.md (related: #972), so I'm going to mark this as Approved, hope that's okay.
| }; | ||
|
|
||
| github.isSignedIn = function() { | ||
| var token = storage_get("oauth_access_token"); |
There was a problem hiding this comment.
nit: Maybe worth extracting a getToken() helper?
We call storage_get("oauth_access_token") a few times, which might be simpler to maintain if handled in one place.
| } | ||
| }; | ||
|
|
||
| var githubURL = "https://api.github.com/gists"; |
There was a problem hiding this comment.
nit: Maybe worth defining this at the top, similar to OAUTH_CLIENT_ID.
| done(null, "HTTP Error "+ githubHTTPClient.status + " - " + githubHTTPClient.statusText + ". Try giving puzzlescript permission again, that might fix things..."); | ||
| } else { | ||
| done(result.id, null); | ||
| } |
There was a problem hiding this comment.
nit: This block of if statements looks similar to the one in load() -- possibly worth extracting a helper like handleGithubResponse().
First, move all the GitHub code into its own
js/github.jsfile. Both the player and editor can use the same basic code for loading gists.Second, support using the token if it’s available. If there’s ever a 401 error when loading or saving a gist, remove the token and tell the user. In the case of loading, it might just be enough to reload (now that the user is signed out, the gist with be loaded with the non-token path).
Detect when there is a rate-limit error loading the game. Tell the user they can try signing in to the editor. For diagnostics, print the rate limit header info to the console log (it looks like:
Rate limit used 15/5000 (resets 2022-09-04T15:30:38.000Z)).This was previously implemented in #606 but later reverted in #689 because it didn’t handle 401 errors.