-
Notifications
You must be signed in to change notification settings - Fork 938
Port 3P token API to app-check-exp #5069
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
Conversation
|
70bc0ff
to
dfbe67c
Compare
} else { | ||
// Otherwise return the token, whether or not it has an error field. | ||
observer.next(token); | ||
} |
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.
It is not correct for 3p listeners - If there is no error handler, the error should just be ignored.
Do we handle both 2p and 3p listeners in this function? If so, they need to be handled differently, where 2p listeners always get the token regardless of the error state, and 3p listeners get a real token in the success handler, and errors in error handler and errors should be ignored if there is no error handler.
That also means we need a way to tell 2p and 3p listeners apart.
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.
I probably missed it in the PR for v8, but we need to do the same in v8 too.
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.
I added a type
property to the observers. Let me know if it works, and if the names are ok.
Changeset File Check
|
Porting 3P token APIs that were added to v8 (non-modular) API in #5033
Also added some test reliability improvements.