Skip to content

feature/SCKT-3_Adding_error_messages_object_for_422_status_code #4

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MostafaFawzy7
Copy link
Contributor

No description provided.

this.unauthorizationCallback();
}
if (this.globalErrorCallback) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be elseif ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right, I started with like this at 1st but I changed it accidentally later!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! ✅

this.unauthorizationCallback();
}
if (this.globalErrorCallback) {
this.globalErrorCallback();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might need to pass the error object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't get passing error object where...?

this.unauthorizationCallback();
} else if (this.globalErrorCallback) {
this.globalErrorCallback();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this

Suggested change
this.globalErrorCallback();
this.globalErrorCallback(error);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a callback doesn't accept any parameter why should I pass the error object to it?!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MostafaFawzy7 to let the consumer know what exactly happened. Give the consuming app some context. This is actually a pretty good idea.

Copy link
Contributor Author

@MostafaFawzy7 MostafaFawzy7 Mar 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HassanElZarkawy The idea of this callback is to add a custom error by the consumer at the global scope of error handling, why to pass to it the error object which is already exposed by the Promise rejection at the next line of this condition?

@Abdallah-Abdelkhalek
Copy link

Did you test it locally by linking?

@MostafaFawzy7
Copy link
Contributor Author

MostafaFawzy7 commented Mar 16, 2024

Did you test it locally by linking?

Why? Do you have an issue with it? it's just a condition to invoke the new callback...

@supaapps supaapps deleted a comment from HassanElZarkawy Mar 17, 2024
@Abdallah-Abdelkhalek
Copy link

Why? Do you have an issue with it? it's just a condition to invoke the new callback...

I didn't test it and was asking if you did

@MostafaFawzy7
Copy link
Contributor Author

@MostafaFawzy7 to let the consumer know what exactly happened. Give the consuming app some context. This is actually a pretty good idea.

@SirNarsh @HassanElZarkawy @amrography Done as we agreed on this finally on the standup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants