Skip to content
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

Remove callback API #285

Closed
wants to merge 3 commits into from
Closed

Remove callback API #285

wants to merge 3 commits into from

Conversation

freitagbr
Copy link

Fixes #246

Deprecates the callback API, leaving only the synchronous API. The methods exposed by this package are synchronous internally, so there is no benefit in making them asynchronous.

@graingert
Copy link

@ziluvatar wasn't this going to go into 8.0.0?

@ziluvatar
Copy link
Contributor

ziluvatar commented Sep 6, 2017

I decided to leave it out for now and package the other breaking changes alone. I am still thinking about this change and the suffering we will generate for people to migrate (easy change from our point of view, but each consumer has its own business). The same way the callback api is useless on verify (nowadays) removing it gives more pain than benefit to current consumers.

I also have a pending PR where having a callback will make you possible to add some verifications that could require asynchronous operations.

Summary: leaving the API with callback remove possible migration pain and leaves open the door for future async verifications. Nevertheless I don't close this issue because it's still something we could apply (not decided yet).

@ziluvatar
Copy link
Contributor

Finally we took advantage of the callback on verify that will behave async if needed by the consumer.
See: #480

I'm closing this PR. Thanks @freitagbr for the effort even if it did not get merged.

@ziluvatar ziluvatar closed this Jun 11, 2018
@freitagbr freitagbr deleted the remove-callback-api branch July 12, 2018 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants