@W-16553557 Add passwordless login helpers #173
Conversation
| grant_type: 'client_credentials', | ||
| hint: 'pwdless_login', |
There was a problem hiding this comment.
These parameters come from the API docs: https://developer.salesforce.com/docs/commerce/commerce-api/references/shopper-login?meta=getPasswordLessAccessToken
src/static/helpers/index.ts
Outdated
| @@ -8,6 +8,7 @@ | |||
| // Doing so may lead to circular dependencies or duplicate exports (due to rollup mangling the types) | |||
| export * from './environment'; | |||
| export * from './slasHelper'; | |||
| export * from './passwordlessHelper'; | |||
There was a problem hiding this comment.
NIT: The passwordless login API's are part of SLAS right? And from the looks of it, the content of this file is somewhat small, is there a reason you didn't slap it in the slasHelpers, sounds like that is the right context for it.
There was a problem hiding this comment.
Yes, but it was moved to its own file because adding these functions fails the 500 line rule for slasHelpers in linting
There was a problem hiding this comment.
Thats super lame, seems a little arbitrary.
It definitely seems like the SLAS passwordless helper should be located along with all the other SLAS helpers. Can you propose an alternative (non-breaking) solution here?
There was a problem hiding this comment.
Agreed that it's pretty arbitrary, when the line limit was implemented (3 years ago) I don't think the team originally envisioned a helper file growing to this size. I think it'd be ok to update the line limit or ignore it for this file:
Ignore: /* eslint-disable max-lines */
Update max lines:
commerce-sdk-isomorphic/.eslintrc.json
Line 57 in 2af2671
There was a problem hiding this comment.
I'll override the max-lines for this file to make it as flexible as possible for new helpers as part of this epic
| )}`; | ||
| const tokenBody = { | ||
| user_id: parameters.userid, | ||
| mode: parameters.mode, |
There was a problem hiding this comment.
NIT: It doesn't look like we have have a pattern for throwing when required params are not included, so I don't expect you to add it now because I like consistency more. But I'll take note on that.
There was a problem hiding this comment.
It also seems like typescript checks for the existence of these params that are directly required. But when it's parameters within an object like ShopperLogin, typescript doesn't automatically give an error that expected arguments don't exist
There was a problem hiding this comment.
We shouldn't reply on TypeScript types for things like that because this lib is compiled into JS and it can be used by a project that isn't TS. Typescript won't bake in any kind of parameter checking so there is the possibility that you this code would make a server request that would for sure fail if that param is required.
But again, no need to change that now, it doesn't seem like it's a pattern that we have, and we should probably fix that in another PR in one big swoop.
| siteId: string; | ||
| }>, | ||
| credentials: { | ||
| clientSecret: string; |
There was a problem hiding this comment.
You might want to throw if this information is not provided as we are doing here. This comment also applies to the getPasswordLessAccessToken function implementation below.
Adding login helpers in isomorphic for passwordless login
E2E Test:
passwordless/login.