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

Why is the "shippingType" member of PaymentOptions not a PaymentShippingType? #337

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

Comments

@bzbarsky
Copy link

The spec has a note about replacing invalid values with "shipping" is supposed to let a page detect when it's using an unsupported shipping type, but an exception from the dictionary conversion would be just as detectable, no?

@domenic
Copy link
Collaborator

domenic commented Dec 13, 2016

@zkoch @adrianba thoughts? Throwing seems a lot nicer than silently changing it.

@zkoch
Copy link
Contributor

zkoch commented Dec 13, 2016

Is this referring to these particular steps:

Let shippingOptions be the sequence details.shippingOptions.
If the shippingOptions sequence contains multiple PaymentShippingOption objects that have the same id, then set shippingOptions to the empty sequence.
If the shippingOptions sequence contains a PaymentShippingOption that has an amount.value that is not a valid decimal monetary value, then set shippingOptions to the empty sequence.
Copy shippingOptions to the shippingOptions field of target.[[details]].

I'm trying to recall the discussion we had around this. I can't quite recall why we didn't throw. Reading this again, throwing seems good to me. @adrianba do you recall?

@domenic
Copy link
Collaborator

domenic commented Dec 13, 2016

No, it's referring to

If options.shippingType is not a valid PaymentShippingType value then set the shippingType attribute on request to "shipping".

Note
This behavior allows a page to detect if it supplied an unsupported shipping type. This will be important if new shipping types are added to a future version of this specification but a page is run in a user agent supporting an earlier version.

@zkoch
Copy link
Contributor

zkoch commented Dec 13, 2016

Ahhh. This is what happens when I don't read the issue title closely enough. 😨

Throwing seems pretty serious for what is effectively a string in the UI.

@domenic
Copy link
Collaborator

domenic commented Dec 13, 2016

I mean, it seems better than displaying "shipping" when you meant "pickup" (but you mistyped it as "pick-up"). You can imagine some irate customers waiting for their package to ship for weeks, not knowing it's waiting at the store for pickup...

@domenic
Copy link
Collaborator

domenic commented Jan 20, 2017

Can we get a decision on this, one way or the other? I think using an enum and following the usual platform enum rules makes more sense than creating a bespoke behavior here because we're afraid of being too serious. But it doesn't really matter much, we should just make a decision so we can close the issue.

If we decide against using an enum we should add a note to the spec saying that this is an intentional violation of the usual platform semantics in order to be more tolerant of mistakes.

@ianbjacobs
Copy link
Collaborator

Is this a topic that deserves attention on a WG call?

Ian

@zkoch
Copy link
Contributor

zkoch commented Feb 9, 2017

Chatted with Domenic about this. Decided throwing is best for clarity and future use cases.

domenic added a commit to domenic/browser-payment-api that referenced this issue Feb 9, 2017
This allows better future extensibility, including for proposals such as w3c#409 which give shippingType new meaning.

Closes w3c#337.
marcoscaceres pushed a commit that referenced this issue Feb 10, 2017
This allows better future extensibility, including for proposals such as #409 which give shippingType new meaning.

Closes #337.
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

No branches or pull requests

4 participants