-
Notifications
You must be signed in to change notification settings - Fork 387
Change beginAuth sessions default to online #203
Conversation
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.
LGTM, with one nit: we should update the docs page for this method too, to make sure the param is documented, and so is its default value.
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.
Awesome! Let's not merge this yet until we've had a chance to make all of the breaking changes so we can release it all at once.
Shopify's style guide prefers avoiding terms like "white-list" and "black-list". Drop in replacements include "allow-list" and "deny-list", or "clearlist" and "blocklist". Also consider if there is a domain specific term you could use. For example, "parameter white-list" could be replaced with "parameter allow-list", but "trusted parameters" may more suitable. |
0779b13
to
6287278
Compare
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.
LGTM, just a nit on the docs.
Register webhooks section (Express) loadCurrentSession method uses `request` and `response` props instead of `req` and `res` as returned in the callback
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, some minor nit picks.
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.
LGTM - one small note
WHY are these changes introduced?
Change the default for
OAuth.beginAuth
to online sessions to match the default forloadCurrentSession
.WHAT is this pull request doing?
beginAuth
'sisOnline
parameter to true for online access tokensType of change
Checklist