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

Add progress to Q.all #782

Open
wants to merge 7 commits into
base: v1
Choose a base branch
from
Open

Conversation

darkdragon-001
Copy link

This change adds progress notification to Q.all().

@kriskowal
Copy link
Owner

This change will need to be cast in a way that does not break backward-compatibility, by preserving existing properties.

@darkdragon-001
Copy link
Author

@kriskowal I removed the {index: i, value: v} notifications so that one does not have to check which notification type is currently thrown. I think the number of resolved elements is way more meaningful than the progress of every single element.

When I readd them, the old notifications will still be thrown the same way as before, but the original tests will still fail since additional notifications are thrown.

Which behavior do you prefer? What do you suggest?

@kriskowal
Copy link
Owner

For context, I thought hard about this API and removed it entirely in the v2 branch in favor of an "estimated time to completion" observer, which has much more sensible composition semantics through "any" (min) and "all" (max). So what I want in the v1 branch isn’t particularly relevant, but the semver contract with our users is that no minor or patch release will break existing usage.

So, whatever you change here has to be additive, not subtractive.

@kriskowal
Copy link
Owner

…and the best way to guarantee it is additive is to add tests, no updates or removals.

@darkdragon-001
Copy link
Author

... since the current version of tests compares all received notifications with an expected result, there is no way of adding additional notifications since it won't be equal to the previously expected list any more!?

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.

2 participants