-
Notifications
You must be signed in to change notification settings - Fork 40
Changes request patterns and response handling to match Go backend. #253
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0fe8e51
to
9ed1912
Compare
Lord forgive me, work on this branch was getting so stale that I just went and redid it for a third time. It actually works now though. Unblocked. |
willob99
reviewed
Oct 29, 2020
willob99
reviewed
Oct 29, 2020
user does not yet exist before creation in getUserData.
Add support for React/JSX in the editor
willob99
approved these changes
Nov 13, 2020
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.
Looks good to me!
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Updates the frontend's backend request pattern to match our Go backend's.
ok
field in backend responses).GET
for getting your user data, andPUT
for putting new user data in the database)userCreate
function tofetch.js
(see here).getUserData
This is the weird thing. The JS backend, believe it or not, never had a
createUser
endpoint. Instead, it just made a call togetUserData
and then thegetUserData
endpoint would create the user document if it didn't exist??? You can check out the source code for yourself. This is real.There's a lot to unpack there, so let's dive in.
Why?
Well, if you take a peek at our user creation flow, specifically on line 111, you'll see that we make a direct call to Firebase to create the user.
Is this wise? Kind of. It lets us get away with user creation without having to deal with hashing and storing it ourselves. The downside, though, is that we now have a promise-chained form submission that touches the database directly. I tried splicing a direct call to the
user/create
endpoint on the new backend in there (say, in thethen
after the call to firebase), but to no avail.(less-than-ideal) Solution, Risks & Trade-offs
So how do we solve this problem? The fix I used was a rather dicey maneuver: replicate the JS backend's behavior, but on the frontend. That is, if someone has a cookie for an account that doesn't exist, and makes a call to
getUserData
, then on failure the function will try to make a call tocreateUser
before recursively calling itself. This is """"technically"""" safe, sincecreateUser
will fail if the backend is down, and succeed if the user already exists, but we can hypothetically get into an infinite recursion situation in the perfect storm.I personally don't like that, and would like to eliminate this artifact, but I think that at this point we've been sitting on this PR for 6 months. It's "view-only" (#219) round two. It's time for it to get merged and handle the issue in another one.
This PR supersedes #235, and will hopefully - in time - archive uclaacm/TeachLAJSBackend. It will warrant a major semver bump, and deprecation of the TeachLAJSBackend. Prior to merge, the production Go backend will be deployed on Heroku.