-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
bc4ca90
to
77b01ec
Compare
Isn't there some lightweight ping pong like API which can confirm functionality? Occasionally I've seen huge latency with metafields fetch. |
Since we're testing authentication, no I don't believe so. |
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.
Makes sense to me!
if (e instanceof HttpResponseError && e.code == 401){ | ||
// only catch 401 errors | ||
} else { | ||
throw e | ||
} |
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 (e instanceof HttpResponseError && e.code == 401){ | |
// only catch 401 errors | |
} else { | |
throw e | |
} | |
if (e instanceof HttpResponseError && e.code != 401) { | |
// only throw non-401 errors | |
throw e | |
} |
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.
For the lines just above this try
block, there are identical lines in a PR by @carmelal
https://github.com/Shopify/shopify-node-api/blob/597c870978c9b6cb9b187ea240e1841f0bfd062a/src/auth/session/session.ts#L33-L34
I'm wondering if this change should wait for that other one to be merged?
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 would only catches HttpResponseErrors, but I'm sure there could be more error types returned?
Oh, once carmela's PR is in, we could grab IsActive and use it here? Sounds good to me.
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 Carmela's changes are updating this same place, so we just need to make sure they're merged properly.
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 also have a question about this block, which I think relates to Kevin's suggested change:
Since there is nothing in the if
block, won't this not do anything with the error when it is a 401
? Right now we only seem to throw
in the else
. Or is the intention for it to error silently in those cases (401
)? Sorry if I'm missing something obvious 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.
Yes! The intention is to error silently only on 401s
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.
The 'silent error' here means we proceed as normal when there is no session - i.e. if the session expired we go to /auth
.
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.
Ah ok! Then this works for me! Sorry I misunderstood the intention 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.
This looks good! Gracias for explaining the silent error situation.
WHY are these changes introduced?
Fixes #90
WHAT is this pull request doing?
We need to ensure that oauth is fully functioning, so we can make a call to metafields to ensure oauth has succeeded. This was the approach that we used to implement Shopify/quilt#940
*Need to fix tests
Type of change
Checklist