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

ObservableArray without assignment #1027

Closed
travisleithead opened this issue Oct 4, 2021 · 12 comments
Closed

ObservableArray without assignment #1027

travisleithead opened this issue Oct 4, 2021 · 12 comments

Comments

@travisleithead
Copy link
Member

As a follow-up to my comment here, and as a compromise position, I'd like to make one last push to make assignment on an ObservableArray attribute throw.

The downside to the current behavior (as noted) is that JavaScript developers might not be aware of the copy semantics that assignment has, and will assume that the Put results in changing the ObservableArray to refer the the same instance that was assigned (and that subsequent updates to that instance will be reflected in the ObservableArray). To avoid potential confusion, having assignment throw will be sure to prevent any failed expectations. (And such an artificial restriction can be lifted in the future without back-compat concern if necessary.)

There seemed to be at least some support for this idea in the linked issue thread.

Caveat: such a change would not be backwards-compatible with the adoptedStyleSheets property, which depends on assignment today to change/update the set of adopted sheets.

I'm interested in seeing a mutually acceptable compromise between supporters and objectors--if this proposed change can be that compromise that leads to general agreement on the behavior for ObservableArray among implementers, then I view the loss of good ergonomics as acceptable. The alternative is the status quo, now merged into CSSOM for adoptedStyleSheets which I'm happy to support if compromise cannot be reached.

Related folks: @rniwa, @emilio, @annevk, @mfreed7, @domenic

@annevk
Copy link
Member

annevk commented Oct 5, 2021

I tend to think people will find out quickly enough what kind of semantics the setter has, but I do think #485 would help here from a Web IDL perspective by allowing us to make it more clear that the setter essentially takes a sequence and therefore does not preserve identity.

Aside, the CSSOM description doesn't seem to describe the getter and setter?

@EisenbergEffect
Copy link

EisenbergEffect commented Oct 5, 2021

tl;dr;

  • I need some version of this feature.
  • Please don't break my already deployed code.

As a library implementor, if I were starting from zero today, I would't have a lot of strong opinions about what the API looked like in its final form. I would want the adoptedStyleSheets feature to implement against, but would abstract that away anyways. Such an abstraction could handle any property name, it could handle whether there's a setter or not, it could use add/remove APIs. It's trivial to adjust to any of these combinations while still providing the exact same ergonomics and dev experience to creators using my library (at least with the design we have).

However, we've got a library in production for 18mo or so, upon which hundreds of components have been built, used internally at Microsoft by at least several hundred engineers, deployed on major sites, that if broken will cause substantial damage. You could argue that we shouldn't have used this API until these details were locked down. At the time there were substantial benefits to doing so, and there was precedent for its use among nearly all major library authors. I had myself been using it prior to Microsoft's adoption of the API, for at least a year IIRC.

For better or worse, this is a reality we have to deal with. We are not the only ones in this situation. In light of this, I have a couple of requests:

  • I need a way to reliably detect which version of this feature a given browser has currently implemented. I'd prefer not to have to test an API to see if it throws in order to determine the feature version.
  • I need some way to prevent existing deployments from completely breaking when a browser ships the new behavior. We cannot assume that it's possible to have upgraded any deployed versions of our library in lock step with a Chromium release.

Because of these needs, at this point in time, I'd prefer to either:

  1. Keep the property setter operational OR
  2. Change the property name entirely

If we go with Option 1 then I don't need to change any code. If we go with Option 2 then my code changes are minor. I can easily feature detect without exceptions being involved. Because our existing implementation already feature detects for adoptedStyleSheets, in the case of a name change and a removal of the old property, our existing deployed libraries will continue to work since they will fallback to use code for browsers without adoptedStyleSheets. That is acceptable during an interim period while libraries are being updated.

The problem with keeping the name the same and then making the setter throw is that existing libraries which may not be upgradeable in time, will all the sudden start to fail. I'd like to avoid that most of all.

For reference, the feature detection that has been deployed for adoptedStyleSheets is as follows:

Array.isArray(document.adoptedStyleSheets) && "replace" in CSSStyleSheet.prototype

It now makes me wonder, does Array.isArray(...) return true or false for an ObservableArray? I assume true but if it actually returns false then perhaps there is no issue at all, at least for our library. It seems to me that would cause a different set of issues, perhaps more serious though...

@domenic
Copy link
Member

domenic commented Oct 5, 2021

It returns true.

@travisleithead
Copy link
Member Author

Thanks @EisenbergEffect. Indeed changing the behavior of adoptedStyleSheets in a non-backwards compatible way is a non-starter. Completely agree with options 1 and 2 for adoptedStyleSheets.

I understand that so much of this debate is tied up in the specific use of ObservableArray in adoptedStyleSheets. I would want to retain compatibility there no matter what. My use case is actually in FormattedText's textruns property, where I wonder if assignability conveys the wrong expectation about the textruns property identity. @EisenbergEffect, given your experience, I would love your thoughts on whether this is an imagined problem in general? Do (or would) authors assume that an assignment preserves the identity of the array instance? Or as @annevk asserts above, will people quickly figure out (how?) what kind of semantics the setter has?

@EisenbergEffect
Copy link

I just want to make sure I'm answering the right question. Are you saying that you want to know if this is surprising or not?

const myTextRuns = [];
const text = new FormattedText();

text.textruns = myTextRuns;

assert(myTextRuns !== text.textruns);

@domenic
Copy link
Member

domenic commented Oct 5, 2021

Aside, the CSSOM description doesn't seem to describe the getter and setter?

ObservableArray attributes don't have getter or setter steps; instead their getter/setter algorithms are auto-generated by Web IDL in the attribute getter/attribute setter algorithms at https://webidl.spec.whatwg.org/#define-the-attributes .

It might be nice if there were some Bikeshed spec mechanism so that the attribute in the IDL block auto-linked to something in the Web IDL spec, so that it doesn't seem like the attributes have no definitions.

@travisleithead
Copy link
Member Author

[...] Are you saying that you want to know if this is surprising or not?

assert(myTextRuns !== text.textruns);

Yes, exactly. If this is not really a surprise, then the current behavior won't be a problem, and this issue is moot.

@EisenbergEffect
Copy link

EisenbergEffect commented Oct 6, 2021

I thought about this for a bit...and it's not so easy to answer.

On the one hand, when it's expressed as I have above, and if I'm thinking in terms of vanilla JS, I do find that code surprising. However...I don't personally write that kind of code. So, I'm not likely to be affected by it. I can see how some hard-to-track bugs could come from that if you don't have unit tests covering your mutation patterns. Perhaps this is what @annevk means by "people will find out quickly enough what kind of semantics the setter has".

On the other hand, after reading the WebIDL docs on ObservableArray (thanks @domenic !) and referring back to @annevk's comment, it does make sense to me. I have no issues with that.

So, a bit of a mixed bag, but I'm leaning towards this not being a big issue. Perhaps potential problems can be handled with documentation? Make this very explicit not just in improvements to WebIDL, but also call this out in places like MDN and even in TypeScript d.ts. docs. At least with FormattedText I'm inclined to think that folks using that API are going to spend time investing in learning the best API patterns. That's not something that the average web dev is going to be messing with.

What other APIs are slated to be switched over to ObservableArray? What kinds of patterns in the wild are used with those APIs today?

@mfreed7
Copy link

mfreed7 commented Oct 6, 2021

The potential concerns raised here sound like they are pretty much the same as the ones discussed previously. And as before, there are ways to argue both sides. Since the previous discussion also came to a consensus (WICG/construct-stylesheets#45 (comment)) with resulting landed spec update, I think it only makes sense to conclude the same in this case and close this issue as no-change.

What other APIs are slated to be switched over to ObservableArray? What kinds of patterns in the wild are used with those APIs today?

This is a great question. One potential answer is that adoptedStyleSheets has been implemented in Chromium for several years now, with usage amounting to 13% of all shadow DOM usage, and it uses an assignable interface exactly as is proposed. However, the bug tracker shows zero reported bugs about the copy semantics of an assignable adoptedStyleSheets. While this doesn't mean there aren't people getting confused by this aspect of the feature, it does mean that none of their confusion has risen to the level it takes to report a bug. Perhaps the fears about developer confusion are just hypothetical? Certainly good documentation can greatly help as well.

@travisleithead
Copy link
Member Author

Alright, I think I am convinced that any developer confusion can be addressed in documentation, tests, or practical experience with the type. Thank you all for participating and again hearing this concern. I look forward to seeing implementations of this type land soon.

@othermaciej
Copy link

othermaciej commented Nov 9, 2021

The legacy behavior of adoptedStyleSheets is one thing, but it seems deeply weird that ObservableArray has built-in copy-on-assigment semantics defined at the Web IDL level. But maybe it's less weird than I think. Is there any other Web IDL type where that's true?

That said, it's possible for other specs (even adoptedStyleSheets or some renamed form of it, depending on how that debate works out) to define ObservableArray attributes as readonly, which avoids the potentially strange-seeming behavior (there's no sensible assign-by-reference semantics in cases like this).

@domenic
Copy link
Member

domenic commented Nov 10, 2021

Is there any other Web IDL type where that's true?

FrozenArray<> is the biggest example, used (with such a setter) in the accessibility object model and in the media session API. We make a copy of the incoming iterable object into an array, freeze the resulting array, and then start returning that.

Arguably this is true for all "primitive" Web IDL types, e.g. if you do

document.title = { toString() { return "foo"; } };

this will perform a "copy-on-assignment" of the right-hand side onto the internal title property, and document.title's getter will return a serialization of the result. I appreciate it's a bit different since the "normal" use case for primitive setters is to assign a correctly-typed primitive to the right-hand side; in such a normal use case you could either make an argument that the behavior is copy-on-assign, or that it's storing a reference, since the === operator gives you no way to distinguish.

A variant is how numeric properties are treated, i.e.

element.tabIndex = 1.1;

will perform a "normalizing copy" of 1.1 into 1, so that el.tabIndex will return 1 and not the original 1.1. Again, not quite the same, but IMO similar in spirit.

Of the remaining types that can be used as the type of a settable attribute in Web IDL, we have:

  • object/symbol/interface types/callback interface types: usually treated as opaque references, with no copies involved. (Unless something special is going on with a particular object usage, like is done in HTML's valueAsDate.)
  • Promise<T>: nominally has copy semantics, but in practice I don't believe anything on the platform uses settable promise attributes, and I feel like we might have some issue filed somewhere to disallow them being ever introduced?
  • buffer source types: kind of unclear; the Web IDL spec does references for the basic conversion algorithm, but encourages specs to make a copy of their contained bytes when possible. This is also another case where settable attributes of this type are rare, although I do see one in Web RTC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants