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.

Sign up for GitHub

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

Jump to bottom

updateWith will silently ignore data in hard-to-debug ways #350

Closed
bzbarsky opened this issue Nov 30, 2016 · 3 comments
Closed

updateWith will silently ignore data in hard-to-debug ways #350

bzbarsky opened this issue Nov 30, 2016 · 3 comments
Assignees
@domenic
Labels
Core Functionality
Milestone
CR

Comments

@bzbarsky
Copy link

There are various cases in which updateWith will ignore the data its argument promise is resolved with. For example, it will ignore various negative amounts, onvalid decimal monetary values, etc.

When this happens, there is no indication to the caller of updateWith that anything went wrong. Should there be (e.g. via a Promise that updateWith returns)? I assume UAs are free to report things to the console here, of course, but that only helps if the page author is reproducing the problem themselves...
@domenic
Copy link
Collaborator

domenic commented Dec 13, 2016

If this returning a promise idea were desired, it'd be a slight breaking change, as all the synchronous error throwing would become rejected promise-returning.

@marcoscaceres
Copy link
Member

@domenic, given that it returns void right now, would it be that harmful to start returning a promise?

@domenic
Copy link
Collaborator

domenic commented Feb 8, 2017

I don't think so, but who knows, maybe people are try-catching the errors it produces.

domenic added a commit to domenic/browser-payment-api that referenced this issue Feb 9, 2017
@domenic
Make updateWith() return a promise signaling any update failures
27eab17 
This closes w3c#350. It is a slightly backward-incompatible change in that consumers which were previously try-catching errors will no longer see them. But consumers that were always passing valid data, or passing invalid data and not try-catching the errors, will see no changes.
@domenic domenic mentioned this issue Feb 9, 2017
Closed
domenic added a commit that referenced this issue Mar 8, 2017
@domenic
Make updateWith() reject the show() promise on validation failures
41ef0f3 
Previously, it would ignore any bad data. Now it validates the data ahead of time before performing any updates.

Closes #350.
@domenic domenic mentioned this issue Mar 8, 2017
Merged
@zkoch zkoch added this to the CR milestone Mar 13, 2017
zkoch pushed a commit that referenced this issue Mar 22, 2017
@domenic @zkoch
Make updateWith() reject the show() promise on validation failures (#451
15f9fc8 
)

* Make updateWith() reject the show() promise on validation failures

Previously, it would ignore any bad data. Now it validates the data ahead of time before performing any updates.

Closes #350.

* Also disallow negative modifiers in updateWith

* Update the model for the "error" member to match discussion

* Move the negative check to the correct spot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@domenic domenic

Labels
Core Functionality
Projects
None yet
Milestone
CR
Development

Successfully merging a pull request may close this issue.

Make updateWith() return a promise signaling any update failures domenic/browser-payment-api
4 participants
@zkoch @domenic @marcoscaceres @bzbarsky