Invalid Web IDL: Sanitizer.config attribute type cannot be dictionary

[tidoust]

A recent commit switched config to be an attribute getter. However, that is forbidden in Web IDL: "the type of the attribute [...] must not be a nullable or non-nullable version of [...] a dictionary type".

(I suspect one reason for the rule is that devs expect attributes to return the same object, as in obj.config === obj.config, and that cannot easily be enforced when a readonly attribute returns a dictionary)

[otherdaniel]

Right. This was the reason for originally doing it as methods. However, we've repeatedly been told that this is bad API ergonomics, and that developers would expect to use attribute syntax (s.config instead of s.config()).

At this point, I'm rather confused whether I'm using the syntax wrong -- that is, we can get our attribute syntax for .config, but not with an actual attribute -- or whether we're just caught between two different expectations (accessor syntax vs. object identity for accessor values) and get to pick which one we violate.

Can I return a dictionary copy and get accessor syntax in WebIDL? How?

[dontcallmedom]

a couple of non-fully-formed thoughts:

• getConfiguration() as a method name would probably avoid some of the pushback on config() and has precedents in MediaKeySystemAcces and RTCPeerConnection
• there is a related WebIDL discussion which (indirectly) itself points to ReadOnlyArray as a possible solution, possibly already achievable through ObservableArray?

Paging @LeaVerou since this relates to the feedback she conveyed on the spec.

[tidoust]

Related TAG design principle: Attributes should behave like data properties which says "Ensure that obj.attribute === obj.attribute is always true. Don’t create a new value each time the getter is called".

[LeaVerou]

Related TAG design principle: Attributes should behave like data properties which says "Ensure that obj.attribute === obj.attribute is always true. Don’t create a new value each time the getter is called".

Does .config create a new value every time the getter is called?

[otherdaniel]

Related TAG design principle: Attributes should behave like data properties which says "Ensure that obj.attribute === obj.attribute is always true. Don’t create a new value each time the getter is called".

Does .config create a new value every time the getter is called?

I don't think WebIDL has an opinion on that, since WebIDL is convinced this doen't exist.

The 'naive' implementation (at least in Chromium) would indeed create a new value each time, but that could also be implemented differently by memo-izing the value once and then returning the same value each time

[dontcallmedom]

re memoizing, whatwg/webidl#981 (comment) alludes to a would-be [Cached] extended attribute (but also note the concern re mutability)

[otherdaniel]

re memoizing, heycam/webidl#981 (comment) alludes to a would-be [Cached] extended attribute (but also note the concern re mutability)

Oh, right. If we were to return the same dictionary, then that dictionary could be modified by a first caller, and the second caller would then see the same, modified dictionary. That's just a bad idea. So after a second thought, I'm going to say that .config would indeed create a new value every time. Which would disregard the s.config === s.config expectation.

[LeaVerou]

I think it's fine to create a new object when the configuration actually changes, the spirit of the TAG guidance is that you should return the same object if the value hasn't changed.

[otherdaniel]

Well, Sanitizers don't change configurations anyways; configuration is a constructor thing. The point is that JavaScript doesn't have constant objects. I.e.:

const s = new Sanitizer( ...bla bla...);
let c1 = s.config():
let c2 = s.config();
c1.hello = "world";  // Modify the dictionary we received.
"world" == c2.hello;  // true., c1 & c2 are the same object, and thus c2 "sees" the modification of c1

That could be very extra suprising if some part of the code modifies the return value - for example, to build a new configuration - and then another part of the code tries to examine it. Example:

const main_sanitizer = new Sanitizer(...);  // A carefully configured sanitizer for our application.

// Somewhere in libary we want to be slightly more strict.
// Intent: Build a new sanitizer based on main_sanitizer.
// Reality: Have modified main_sanitizer's config.
let config = main_sanitizer.config;
config.blockElements.push("table", "td", "th", "tr");  // We hate tables.
const my_sanitizer = new Sanitizer(config);

// Somewhere else entirely:
let my_config = main_sanitizer.config.  // Oops. Now we're really seeing my_sanitizer's config.

Which is a pity, because the code above looks nice to me. That's how I'd want to use this.

[Sora2455]

JavaScript has frozen objects, but I don't think that resolves the problem here because freezing an array won't stop someone calling push etc on it.

[LeaVerou]

@Sora2455 Yes, it will. You can try it out yourself:

> let a = Object.freeze([1, 2, 3]); a.push(4)
VM188:1 Uncaught TypeError: Cannot add property 3, object is not extensible
    at Array.push (<anonymous>)
    at <anonymous>:1:3

[otherdaniel]

Thanks for bringin up 'freeze'. Frozen is apparently shallow, so Lea's example will no longer work if it's an array as a dictionary value. But I guess we could freeze recursively.

Is there precedent for relying on 'frozen' return values in JS APIs?

[LeaVerou]

Thanks for bringin up 'freeze'. Frozen is apparently shallow, so Lea's example will no longer work if it's an array as a dictionary value. But I guess we could freeze recursively.

Is there precedent for relying on 'frozen' return values in JS APIs?

I can't think of anything for objects, but there's a lot of precedent with lists of values. Here's the long ongoing related discussion.

[domenic]

I'd strongly recommend using a method. There's no precedent for deeply frozen objects in JavaScript or web APIs (which is why the pattern is prohibited by Web IDL).

[domenic]

The other usual alternative is flattening the properties, e.g. instead of using sanitizer.config.allowElements using sanitizer.allowElements or sanitizer.configAllowElements. That looks a bit less suitable here though.

[otherdaniel]

Hmm. So far, I'm not seeing a viable solution, or even consensus on a general direction. I'll revert PR #104 and close out this issue.

Thanks all for the discussion, and please do feel free to re-open this if there's additional info or a good proposal on how to solve this.

[dontcallmedom]

when reverting #104, I would suggest again that getConfiguration() would be better than config()

[otherdaniel]

Reverted. Also renamed the methods to .getConfiguration() / .getDefaultConfiguration().

[LeaVerou]

Coming here late, but +1 for using a function, I think that is the least surprising option.
(to clarify, I wasn't suggesting deeply frozen objects at any point, just answering related questions)