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

Throw on dup shippingOptions ids #596

Merged
merged 11 commits into from
Sep 4, 2017
Merged

Throw on dup shippingOptions ids #596

merged 11 commits into from
Sep 4, 2017

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Aug 25, 2017 •
edited by pr-preview bot
Loading

@marcoscaceres marcoscaceres changed the title Throw on dup shippingOptions (closes #594, ##595) Throw on dup shippingOptions ids (closes #594, ##595) Aug 25, 2017
@marcoscaceres
Copy link
Member Author

Ok, it's Friday... but this strikes me as a pretty big spec bugs. I kept hitting this problem when testing in Chrome, and I thought maybe it was a bug in Chrome... but it's clearly a bunch of spec bugs.

See the linked bugs for example code you can run locally. It shows the problems pretty clearly (specially when you get into the "I can't ship to you" infinite loop where neither the developer or the user know what is going on! 💥 🔥🚨 😱 💩).

@ianbjacobs
Copy link
Collaborator

Thanks, @marcoscaceres for unearthing these points.

@marcoscaceres marcoscaceres changed the title Throw on dup shippingOptions ids (closes #594, ##595) Throw on dup shippingOptions ids (closes #594, #595) Aug 26, 2017
marcoscaceres
marcoscaceres commented Aug 29, 2017
View reviewed changes
index.html Outdated
<var>option</var>.<a>id</a>, then set <var>options</var>
to an empty sequence and break.
<var>option</var>.<a>id</a>, then throw a
"<a>ConstraintError</a>" <a>DOMException</a>. Optionally,
Copy link
Member Author

Choose a reason for hiding this comment

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

should be TypeError

marcoscaceres
marcoscaceres commented Aug 29, 2017
View reviewed changes
index.html Outdated
"PaymentShippingOption.id">id</a>, then set
<var>options</var> to an empty sequence and break.
"PaymentShippingOption.id">id</a>, then <a>abort
the update</a> with a "<a>ConstraintError</a>" <a>
Copy link
Member Author

Choose a reason for hiding this comment

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

as above, TypeError

@adrianba
Copy link
Contributor

As discussed: agree with throwing on duplicate IDs but not on allowing empty shipping options.

@marcoscaceres
Copy link
Member Author

So, we agreed on the developer call to throw on duplicate IDs - but keep "empty shipping options is error"

@marcoscaceres marcoscaceres changed the title Throw on dup shippingOptions ids (closes #594, #595) Throw on dup shippingOptions ids Aug 31, 2017
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 1, 2017
Must throw on duplicate shippingOptions ids
47f0707 
Test for w3c/payment-request#596
@marcoscaceres marcoscaceres mentioned this pull request Sep 1, 2017
Merged
@marcoscaceres marcoscaceres mentioned this pull request Sep 1, 2017
Merged
@marcoscaceres
Copy link
Member Author

Blocked on test review.

marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 4, 2017
Must throw on duplicate shippingOptions ids (#7217)
236959b 
Test for w3c/payment-request#596
@marcoscaceres marcoscaceres merged commit 9e339d0 into gh-pages Sep 4, 2017
@marcoscaceres marcoscaceres deleted the throw_on_dup_ids branch September 4, 2017 06:35
@marcoscaceres marcoscaceres mentioned this pull request Sep 18, 2017
Closed
14 tasks
rachelandrew pushed a commit to rachelandrew/web-platform-tests that referenced this pull request Nov 8, 2017
@rachelandrew
Must throw on duplicate shippingOptions ids (web-platform-tests#7217)
b73e907 
Test for w3c/payment-request#596
jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017
@jakearchibald
Must throw on duplicate shippingOptions ids (web-platform-tests#7217)
5e243a3 
Test for w3c/payment-request#596
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zkoch zkoch zkoch approved these changes

Assignees
No one assigned
Labels
cr-substantive
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Duplicate shippingOptions lead to inconsistent updateWith() - should throw!
4 participants
@marcoscaceres @ianbjacobs @adrianba @zkoch