Add specific error for invalid_target error #4436
Conversation
This comment has been minimized.
This comment has been minimized.
Coverage report
Show files with reduced coverage 🔻
Test suite run success1878 tests passing in 851 suites. Report generated by 🧪jest coverage report action from 9cc4d7c |
| // There's an scenario when Identity returns "invalid_request" when exchanging an identity token. | ||
| // This means the token is invalid. We clear the session and throw an error to let the caller know. | ||
| return new InvalidRequestError() | ||
| } | ||
| if (errorMessage === 'invalid_target') { |
There was a problem hiding this comment.
function tokenRequestErrorHandler(error: string) {
if (error === 'invalid_grant') {
// There's an scenario when Identity returns "invalid_grant" when trying to refresh the token
// using a valid refresh token. When that happens, we take the user through the authentication flow.
return new InvalidGrantError()
}
if (error === 'invalid_request') {
// There's an scenario when Identity returns "invalid_request" when exchanging an identity token.
// This means the token is invalid. We clear the session and throw an error to let the caller know.
return new InvalidRequestError()
}
if (error === 'invalid_target') {
// add a comment here
return new InvalidTargetError(INVALID_TARGET_ERROR_MESSAGE)
}
// eslint-disable-next-line @shopify/cli/no-error-factory-functions
return new AbortError(error)
}There was a problem hiding this comment.
^ can we try keeping these patterns as is?
There was a problem hiding this comment.
@jamesmengo Riley originally had this, but we realised the two custom error types only exist because they are exported and handled upstream. As all we want to do is exit with a message, and we wanted to avoid disabling the eslint rule again, we just wanted to fall back onto the existing AbortError. Another side effect of extending the ExtendableError is that the stack trace is printed out along with the readable error message, which we didn't think was helpful.
Maybe our best option here is create (and not export) a class InvalidTargetError that extends AbortError. This avoids the linting issue of using error factory functions, and keeps the patterns consistent. Also +1 to turning the error message into a constant
There was a problem hiding this comment.
Thanks for the context! That's a great observation that I wasn't aware of :)
As long as the message is in a constant, I'm happy :). I'll save us from having a let vs const debate here since the impact is minimal 😆
There was a problem hiding this comment.
I agree with extracting the message to a constant. And maybe there's no need to create a new error, you could just do something like return new AbortError(INVALID_TARGET_ERROR_MESSAGE).
| 'you should be the store owner or create a staff account on the store.' + | ||
| '\n\n' + | ||
| "If you're the store owner, then you need to log in to the store directly using the " + | ||
| 'store URL at least once before you log in using Shopify CLI.' + |
There was a problem hiding this comment.
| 'store URL at least once before you log in using Shopify CLI.' + | |
| 'store URL at least once before you log in using Shopify CLI. ' + |
There was a problem hiding this comment.
This wording comes from the Ruby CLI, but I think it's ok to stray from 100% parity here to improve this message ever so slightly.
This is non-blocking, and I'm ok with creating a new issue if you want to address that separately
You are not authorized to use the CLI to develop in the provided store.
You cannot use Shopify CLI with development stores if you only have Partner staff member access. To use Shopify CLI on a development store, you must either be the store owner or have a staff account on the store.
If you are the store owner, you need to log in to the store directly using the store URL at least once before logging in with Shopify CLI. Logging into the Shopify admin directly connects the development store with your Shopify login.
There was a problem hiding this comment.
Functionally, this works and looks like it's in the right place to me. Great work!
Most of my feedback are small nits to follow conventions set by the foundations team.
I would recommend asking @Shopify/cli-foundations for a review as well, since this is in cli-kit 🙂.
I don't have too much familiarity with the specific cases in which invalid_target is returned as an error so just wanted to double check that we're not missing any cases when invalid_target may be returned as it seems like the conditions are slightly different from the ruby implementation
cc: @lukeh-shopify
Thanks for these changes!
| // There's an scenario when Identity returns "invalid_request" when exchanging an identity token. | ||
| // This means the token is invalid. We clear the session and throw an error to let the caller know. | ||
| return new InvalidRequestError() | ||
| } | ||
| if (errorMessage === 'invalid_target') { |
There was a problem hiding this comment.
I agree with extracting the message to a constant. And maybe there's no need to create a new error, you could just do something like return new AbortError(INVALID_TARGET_ERROR_MESSAGE).
itsjustriley
force-pushed
the
invalid_target-error-update
branch
from
02c1e24
to
425ecf5
Compare
itsjustriley
force-pushed
the
invalid_target-error-update
branch
from
425ecf5
to
9cc4d7c
Compare
WHY are these changes introduced?
Resolves #271
When a user has an invalid store url, or the wrong permissions, when using theme dev in the cli they currently receive an ambiguous
invalid_targeterror message.WHAT is this pull request doing?
Replacing ambiguous
invalid_targeterror with specific messaging. (Modified an existing error message to work in the pertinent file, added handling for 'invalid_target' specifically).paired with @EvilGenius13 @AribaRajput @lukeh-shopify
How to test your changes?
pnpm buildshopify-dev theme dev --store wrong_storePost-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist