-
Notifications
You must be signed in to change notification settings - Fork 143
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
[css-typed-om]: There's no nice way to represent CSSUnparsedValue as a "list-plus" style object. #239
Comments
WebIDL doesn't say it's restricted to legacy APIs? It does indeed say that you need an indexed getter and a .length property, tho - we can fix that. |
Indexed getters are legacy for all intents and purposes. As they require a proxy to implement and are therefore not really natural. |
(There's a open issue against IDL to get them renamed.) |
Yeah, I think we do want the indexed getter. People keep talking about it as if it were legacy, but we still have no decent solution for a lot of use cases. That said, in this case we could perhaps use an Array-valued property. At least as long as it does not need to be live in any way. If liveness of some sort is needed, then we're back to indexed getters. |
Nothing is live in the Typed OM. This is a serialization (in object form), and to change anything you have to explicitly set it again. |
Yeah, I raised an issue earlier about this maybe just being an Array, but that got closed... @bzbarsky what use cases though? We don't really want new live lists. And if you really need it, just have a method. |
OK. Looks like you can't modify a CSSTokenStreamValue, so you could make it a frozen array, but...
Consider the case here, where you want to represent "iterable stream of tokens" in a thing that you could pass to |
I reopened #221 - it really shouldn't have been closed. I'm OK with an Array, any objections? This definitely should not be live, and a simple representation is best. Does the Array need to be frozen? |
I think people decided they wanted to return frozen arrays where possible. I'm still not entirely convinced it adds value, but it seems to be the pattern we're using for now... @bzbarsky because StylePropertyMap would have to accept an iterable and return an array rather than set/get of an object? Somewhat convincing, though I think the benefits of using a standard data structure outweigh that. (Ideally we'd have immutable standard data structures of course...) |
That depends. Is the idea to accept the new thing by reference or by value? Right now it accepts and stores it by reference, yes? |
The main issue is that CSSTokenStreamValue needs to be a subclass of CSSValue. Making it an iterable and adding the indexed getters/setters seems like the best solution? |
Please don't add more indexed getters/setters to the platform. Just use |
^^ @domenic |
And that goes directly against the common advice of "ugh, fake-list interfaces with getItem()/etc are so shitty". They're widely considered a legacy mistake every place they appear. This is not a happy space to deal with, but it also shouldn't require a fully Proxy just to implement numeric setters. This is JS's fault for not yet exposing a hook for |
No, they're not actually. Talk to your JavaScript team. |
Yeah, there's a reason we didn't add "indexed getters" to Map or Set or IteratorPrototype. |
I mean, this goes against years of advice I'd heard (for example, how the SVG DOM was so horrible in having tons of I'd be more for just making it a pure iterator with no direct access and require casting it to an array if you want that, than adding (Or just getting JS to do the necessary hops to make indexed access not require a full Proxy, because that's dumb.) |
So I'm leaning towards something that looks like
Purely because it seems to thread all the Opinions on display here. If I could have magical ponies then I think CSSTokenStreamValue would just be an array that subclasses CSSStyleValue, but it seems those are somewhat in demand at the moment. As a side note, y'all need some explainers about what current 'best practise' is, preferably one that has been reviewed by the TAG as well as all relevant JS vendor teams... I don't think the current drive-by conflicting assertions approach is working. |
Yeah, this is pretty terrible, but I'm happy to lay a lot of the blame on WebIDL being unmaintained for a while. This stuff has only gotten really important the last few years, exactly when Cam started backing off from editting. Whatever the "right" decision is, it needs to be documented in WebIDL for posterity. |
Right, there are open issues against IDL to mark proxy-creating members as legacy. |
Except that @bzbarsky said:
There's clearly a lot of disagreement here, and we need to hash it out soon. |
I would like to point out I (still) think using a property instead of making the instances be iterable is a good thing because whenever we are going to support new values that currenty are CSSTokenStreamValue/CSSUnparsedValue (for instance a CSS Shapes Typed OM), we would have to make CSSShapeValue itself be an iterable of String for that not to be a breaking change (and that wouldn't make sense to me). If we go the property route, we could move the property to CSSValue (and return a Frozen array containing one item, the cssText, for any value type except CSSUnparseValue which might return a bigger array since it could contain custom property references). In that case, CSSUnparsedValue would be an empty interface whose only purpose would be to clarify that the value has no other type. Maybe (just thinking out loud here) we can actually drop CSSUnparsedValue entirely, and decide to just leave unparsed things be a bare instance of CSSValue directly. I didn't think this through, though, but that looks reasonable at first glance. |
@FremyCompany I don't think your concern applies any longer - CSSUnparsedValue now exists only to extract custom property references. If you have a CSS Shapes value that isn't currently supported, then it'll be a CSSValue with a .toString(). Which is more or less what you suggested :) |
On discussion with Tab we decided to do:
This is a readonly iterable that you need to reconstruct if you want to change. It sucks a bit but preserved typedness. I'm going to commit that but leave this issue open against it. |
@domenic @bzbarsky @tabatkins to get the full range of opinions... |
As we discussed in the past, @annevk, there's currently no way to get anything remotely array-like in WebIDL, for no particularly good reason. Even the plain Fix that in WebIDL, and we can fix this spec. The only other possibility is making our interfaces super janky, so that you have to access a separate attribute/method to actually get the data as an Array. |
How would that introduce jank? |
(Also, if you actually need Array subclasses, we should just get those added instead of continuing with this hack.) |
"Jank" in the "looks terrible" meaning, not the "slow/jittery" meaning. I don't necessarily need Array subclasses. At bare minimum, I need a value iterator; currently, those are broken in WebIDL and force you into being a Proxy. (This is why I said, in one of the IDL issues, that I could work around it by just using a kv iterator and manually setting up the keys as meaningless indexes - that gets me in the neighborhood of the correct behavior that value iterators should have.) Having an Array subclass would be ideal, tho - without it, we can't reasonably make some of these classes mutate-able. Currently we're planning on just accepting that, so that you have to create fresh objects instead; alternately, we could go the terribly ugly route of having .get/.set arrayish methods. |
We neither need nor want "live" mutability. Updates only happen if you call But it's useful (and more performant) to be able to (The "get a value, mutate it, set it back" pattern is especially useful for animations, where you can just reuse a single object that's continually fiddled with, rather than creating garbage at rAF speed.) |
(In other words, I understand you need a Proxy (or ES to get off its ass and finally let us hook the |
Thanks for the clarification, that helps a fair bit. |
The CSS Working Group just discussed Arraylike, and agreed to the following resolutions:
The full IRC log of that discussion
|
One point that might have been missed in the above discussion is that anywhere you accept a
Doesn't seem very accurate, because you do that anyway, I think. |
Right now we don't have
|
Happy to bump this higher up in my priority list for WebIDL. Seems like fixing option 2 is a no brainer. Haven't thought about option 3 at all in terms of what it would imply for WebIDL. New syntax? Etc. Tracking this in whatwg/webidl#345. Grouped these related issue in a dedicated project to see if that helps. |
That seems very developer-unfriendly. In general anything that accepts an iterable should accept a |
The constructors take sequences, yes. I'm talking about the |
I think it'd be very frustrating for authors if they couldn't do |
That is absolutely how we are designing these APIs, yes. It's rarely going to be a generic |
Ok. Then we are so far outside the realm of good API design I am not really planning to spend more time on this. |
The fundamental problem people are attempting to solve here is to do something like:
but without having to parse that string. That basically means the caller needs to provide an object representation of that thing on the right; more or less an AST of what you'd get from parsing the string. In this particular case, it might be possible to elide the CSSTransformValue information (just like it's elided in the string syntax) if we know that the "transform" property takes a list of things. How that plays with other cases, I'm not sure. |
@domenic That's an extremely disappointing way to treat this, particularly since the thing you're talking about is a small aspect of one possible approach to solving this much larger overall problem with WebIDL. You do you tho. |
So anyway, regardless of whether we decide to accept raw Arrays for things that are list-like (in addition to the bespoke objects), we still have the issue that, if the bespoke objects are Array-like but we can't intercept sets, we still need to verify the correctness of the data on every single set, whereas that's unnecessary for every single other object in the entire Typed OM. (On that tangent - we do plan to accept doubles anytime we'd accept a CSS |
Sigh, new example of an Array-like in Typed OM (that I'm currently in the middle of speccing, so not visible yet in the spec): CSSMathValue. This replacement for CSSCalcValue represents a calc() as an expression tree, so (Note that this is why we can't even depend on |
Okay, unblocking: For now, these interfaces need to just use indexed getters/setters. This'll invoke more Proxy machinery than we need/want, but it'll give us the functionality we need for now, and is compatible with the functionality/syntax that a "proper" solution will have. For later, I missed hitting the July TC39 meeting with the proposal for a limited proxy that just lets you intercept gets/sets on indexed properties, so I'll hit the September meeting instead. This proposal had good support when it was first mentioned back in 2009, and should be nice and cheap; once we get that thru, we can change the spec over to use that mechanism instead. There'll be no observable behavior change for authors (because that's the only part of the Proxy functionality we're using anyway), the objects should just get faster to manipulate (once browsers implement the new thing). (Or rather, WebIDL will switch indexed getters/setters over to using this mechanism, instead of full Proxys.) |
Single-type iterables are reserved to certain legacy APIs with an indexed getter. I don't think we want that here.
@bzbarsky
The text was updated successfully, but these errors were encountered: