-
Notifications
You must be signed in to change notification settings - Fork 0
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
Improve handling of declined portings #14
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.
Looks good barring the new onError
signature which might constitute a breaking change and may limit us down the line.
Other than that it LGTM!
porting?: Porting, | ||
errorCode?: PortingEmbedError |
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.
nit: not sure if it's right to make the error
optional. I think that we should always throw an error to avoid having a breaking change for the customer.
P.S. I'd consider bagging all the custom staff into an object so we can add more things in the future without issuing a breaking change.
Something like
onError: (error: Error, meta: Metadata)
where:
type Metadata = PortingEmbedErrorMetadata | OtherErrorMetadata | ...
type PortingEmbedErrorMetadata = {
code: 'portingDeclined' // all should have a `code` field
porting: Porting
}
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.
thank you!! I agree, makes sense 💯 🙏
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.
Just some final touches and we're good!
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 thanks @dharmadeveloper108 !
if (e instanceof ApiError) { | ||
onError(e, { | ||
code: e.code, | ||
porting: subscription?.porting || undefined, |
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.
optional: same thing :)
porting: subscription?.porting || undefined, | |
porting: subscription?.porting, |
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 that's because porting can be undefined or null here
} else { | ||
onError(new Error('Something went wrong.'), { | ||
code: 'unexpectedError', | ||
porting: subscription?.porting || undefined, |
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.
porting: subscription?.porting || undefined, | |
porting: subscription?.porting, |
When a porting is declined, instead of showing a dead end error state in the embed with no available actions the user can take, we are triggering
onError
.onError
to accept metadata withcode
and optionalporting
paramsonError
a required prop to initializePortingEmbed
onError
from the porting form when a porting is declinedGenericPortingDeclined
componentonSupportRequested