When doing CSSStyleValue.parse(), what should throw vs return null?

[tabatkins]

The old (very incomplete) definition of CSSStyleValue.parse() said that things returned null if they didn't parse. I've rewritten it in a proper algorithmic style, and identified three possible error locations. Which should throw, and which should return null? (Currently, the spec throws for all of them.)

1. Property name isn't an ident.
2. Property name is an ident, but not recognized as a CSS property.
3. Valid property name, but property value doesn't match the grammar of that property.

[SebastianZ]

My gut feeling says throw for 1 and return null for 3. I'm unsure about 2 but tend to throw to be able to distinguish that case from 3.

Sebastian

[shans]

I think 2 is more or less the same as 3 - in each case, it's possible that the failure is due to uneven implementation across different browsers.

[shans]

@SebastianZ WDYT? Should 2 be like 3, or distinguishable?

[SebastianZ]

I stand with my previous comment that 2 and 3 should be distinguishable.
This could either be done by letting 2 throw while 3 returns null, as I wrote earlier, or throwing in both cases (with properly distinguishable exceptions).
It's still not a strong opinion, though.

Sebastian

[shans]

Currently specified behavior is:

1. throws a Syntax Error
2. throws a Type Error
3. throws a Syntax Error

This satisfies distinguishability. Remaining question: should 3. return null instead?

[SebastianZ]

This makes 1 and 3 hard to distinguish, so I still say return null for 3. Any other opinions?

Sebastian

[shans]

I'm leaning that way too. @tabatkins ?

[tabatkins]

#1 is definitely a SyntaxError, no question in my mind. I think #2 being a TypeError has strong reasoning behind it.

I don't have strong opinions on #3. I guess null could be okay, but like you said in #305 (comment), #2 and #3 will commonly be due to the same thing (uneven support between browsers), and having them be extremely different (throw vs null) will often be a disservice, I think.

I think I'd rather have them both throw, with whatever error is most appropriate, and solve the "wrong property, or wrong value?" problem more directly. That sounds like an API idea we can work on. I don't think we should smuggle the use-case into which error we emit.

[css-meeting-bot]

The CSS Working Group just discussed #305, and agreed to the following resolutions:

RESOLVED: Be consistent in all thre cases; follow TAG guidelines once they have them

The full IRC log of that discussion

<TabAtkins> Topic: https://github.com/w3c/css-houdini-drafts/issues/305
<fantasai> TabAtkins: When you call CSSStyleValue.parse()
<fantasai> TabAtkins: parses arbitary stirng into type doOM value
<fantasai> TabAtkins: Takes a property name and a value
<fantasai> TabAtkins: Several wasy to do something wrong, not sur ewhethe rto thorw or eturn null
<fantasai> TabAtkins: 1st case is, property is not even an identifier
<fantasai> SimonSapin: If property is a separate arg, not parsed as "prop:val"
<fantasai> SimonSapin: Then do you even need to parse it?
* astearns minute-man added basically an empty comment for the first instance
<fantasai> SimonSapin: For CSS.supports(), we have API with 2 variations
<fantasai> SimonSapin: One with colon syntax, uses CSS sntax
<fantasai> SimonSapin: Other one passes prop and value separately
<fantasai> SimonSapin: In that case you don't parse it
<fantasai> TabAtkins: ...
<fantasai> SimonSapin: Just assume it's an ident
* dbaron deleted the empty comment... but it turns out that that's the danger of using Topic: rather than Github topic:. Perhaps that was a mis-feature?
<fantasai> TabAtkins: Yes, that avoids the issue entirely
<fantasai> iank_: Might want to throw on the empty string
<fantasai> iank_: We throw a SyntaxError on that
<fantasai> SimonSapin: empty string is not valid property name
<fantasai> iank_: It's not a valid function name
<fantasai> SimonSapin: Are we talkig about property names
<fantasai> iank_: First argument in custom paint is an ident, same issue
<fantasai> shane: What does supports() do with empty string?
<fantasai> SimonSapin: supports method, you can give it custom property, in which case name starts with --
<fantasai> shane: There are 3 differnet erro conditions
<fantasai> shane: 1st, not a valid proeprty name. If tha'ts just empty string, sitll
<fantasai> shane: 2nd, valid property name, but doens't exist. Can only happen for non-custom properties
<fantasai> SimonSapin: could treat those the same
<fantasai> shane: 3rd, valid property name, but value's grammar doesn't match
<fantasai> shane: I thought 2 & 3 were the same, but both f them could be that you're simply running on a browser that hasn't built support for that syntax yet
<fantasai> TabAtkins: Taking the property name makes it not an error
<fantasai> shane: Could treat them all the same, and thorw
<fantasai> s/thorw/throw/
<fantasai> Rossen: It would be easier to handle now from user side
<astearns> s/now/null/
<fantasai> Rossen: Rather than wrapping everything in try for parsing all over the place
<fantasai> Rossen: 1 & 2 are more of an error than 3
<fantasai> Rossen: 3 is you'r eparsing a value that you can't prase
<fantasai> shane: But both 2 & 3 could be that you're trying to parse a feature that the browser doesn't yet support
<fantasai> dbaron: It does feel like the more CSS-ish thing ot do, not to throw for unsupported values or properties
<fantasai> dbaron: In normal CSS, they get silently dropped
<fantasai> dbaron: so null seems more sensible
<fantasai> shane: Note that we throw if you set a value that doesn't parse according to your type already
<fantasai> dbaron: If mistyped things throw, then unknown things should throw. I would agree wiht that
<fantasai> philipwalton: My intuition is, you don't want to both try-catch and check for null when you're writing code
<fantasai> philipwalton: I think htere are many cases you want try-catch, so thorwing an error is probably best
<fantasai> shane: Rosen, you were leaning towards null before. What do you think now?
<fantasai> Rossen: Not gonna object, but not gonna love it.
<fantasai> iank_: What's your hesitation
<fantasai> Rossen: More of a philosophical discussion
<fantasai> Rossen: I'm very biased to the APIs we work on on the windows platform
<fantasai> Rossen: We have strict principles on what is an error, how to deal with errors, how to make API that is ergonomic for developers
<fantasai> Rossen: Don't watn to sprinkle try-catch all over, half is error-handling, half is business logic
<fantasai> Rossen: We have strict rules to guide our APIs. Not gonna impose those here.
* astearns what about not throwing, but returning a value that throws an error when you try to read it? Could that be the worst idea?
<fantasai> fantasai: Might be worth having that philosophical discussion
* TabAtkins Worst idea, yes.
<fantasai> fantasai: and codify some principles for our APIs, whether same or different from Windows API principles
* TabAtkins Value that can validly be used, but represents a random property/value.
<fantasai> shane: Inconsistent to return null here.
<fantasai> shane: You then set this value, want to throw an error here
* fantasai missed exaclty the nuance htere
<fantasai> iank_: Does TAG have any guidance?
<dbaron> I think Shane was saying that it's not as inconsistent as he thought it was before.
<fantasai> plinss: We have a document discussing principles for designing APIs, but don't have anything on this topic
<shane> yup. When we set values we need to throw an exception on error because there's no return value
<dbaron> because the other case was setting an object and getting a type mismatch, whereas this is parsing a string.
<fantasai> Rossen [offers to review that? send feedback? something?]
<shane> but here we're getting the result as the return value so we can use null to signal an error
<fantasai> Rossen: So going back
<fantasai> Rossen: Are you reverting your opinion about throwing vs null?
<fantasai> shane: Yeah
<fantasai> shane: I'm going back from saying we should throw to not having a strong opinion
<fantasai> Rossen: Does anyone have a strong opinion?
<fantasai> philipwalton: Likely to do this in environment where you don't know what your'e getting
<fantasai> philipwalton: so maybe throw
<fantasai> Rossen: Think about it from a mid-layer library, start exposing some of those
<fantasai> Rossen: First they're going to write their own wrapper that does a throw-catch
<fantasai> Rossen: are going to call that so they don't have to deal with throw/catch all over
<fantasai> Rossen: If that's the principle we design around, okay
<fantasai> Rossen: ...
<fantasai> fantasai: Sounds like we should table discussion and have your philosophical discussion about throw vs null?
<fantasai> plinss: To me, throwing is semantically different than returning an error
<fantasai> plinss: If you pass in a property that isn't an ident, e.g. numbers, cna't possibly be a rpoperty name. Could see an error
<fantasai> pl	But if passing a name that's unknown to the end�gine, then return null
<fantasai> P������pliNot recommending htat's what we do, but there's a distinction between throwing errors and returning null, and it's useful to make that disu�ction
<fantasai> TabAtkins: If you do construct a value that doesn't make any sense, and you return null. Whatever you do with that will throw an error
<fantasai> TabAtkins: All you can do is check for null
<fantasai> plinss: Throwing is ofr exceptional  circumstances like out of memory
<fantasai> plinss: I've tried using it for everthing, and that way lies madness
<fantasai> plinss: For a certain class of things, throw exceptions, and be consistent about it.
<fantasai> plinss: We need some guidance on that, and we don't have it
<fantasai> iank_: I was just looking through a few of the Web apis, like json.parse
<fantasai> iank_: Dae.parse() doe snot ... seems like more things throw than silently fail
<fantasai> iank_: HTml throws if not well-formed
<fantasai> iank_: Could file an issue on the TAG
<fantasai> shane: Let's do that
<fantasai> Rossen: Let's resolve with following -- in all three cases we will be consistent in how we handle the condition
<fantasai> Rossen: And we will follow the TAG guideline on whether to throw or return null
<dbaron> The TAG guidance might not be all that prescriptive...
<plinss> https://github.com/w3ctag/design-principles/issues/55
<fantasai> RESOLVED: Be consistent in all thre cases; follow TAG guidelines once they have them

[tabatkins]

Let's do TypeError for everything; that's the error we're currently consistently throwing in all the StylePropertyMap similar cases, and they don't even check for whether the string is a valid ident.