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

Instead of assignable FrozenArray, use add / remove #45

Open
rniwa opened this issue Oct 23, 2018 · 28 comments

Comments

@rniwa
Copy link

commented Oct 23, 2018

In cases where this API is used by custom elements, different subclasses may want to add a different stylesheet to adoptedStyleSheets. In that case, it's better to have explicit add and remove methods rather than to have author scripts manipulate FrozenArray.

@domenic

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

We can do both! add/remove would be sugar, in addition to allowing access to the list.

@rakina rakina added the enhancement label Dec 10, 2018

@developit

This comment has been minimized.

Copy link

commented Jan 15, 2019

Just catching up on this - I'm definitely fond of explicit methods. Here's my pitch:

interface DocumentOrShadowRoot {
  void adoptStyleSheet(CSSStyleSheet sheet)
  void removeStyleSheet(CSSStyleSheet sheet)
}
@chrishtr

This comment has been minimized.

Copy link

commented Jan 15, 2019

The methods as described above might not make it obvious enough that there is an ordering to the style sheets, with later style sheets overriding earlier ones for cases when both apply to an element.

@emilio

This comment has been minimized.

Copy link

commented Jan 15, 2019

What about prependStyleSheet / appendStyleSheet / removeStyleSheet? That makes the ordering explicit, and if you want something else you just go ahead and manipulate the list yourself.

@emilio

This comment has been minimized.

Copy link

commented Jan 15, 2019

(Not that I'm particularly fond of it, just dumping thoughts :))

@developit

This comment has been minimized.

Copy link

commented Jan 29, 2019

Perhaps the FrozenArray used here could be extended to allow indirect modifications like CSSRuleList (document.adoptedStyleSheets.insertSheet(sheet, 0)) or NodeList?

I'll admit it seems odd to add a whole new API for working with an ordered set of like objects.

@surma

This comment has been minimized.

Copy link

commented Jan 29, 2019

May I ask what the rational was behind making it a frozen array in the first place? It seems a bit odd to make it a frozen array and then re-implement methods to basically allow arbitrary manipulation.

@domenic

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

It's not possible to make an array that the browser "watches" for changes. So, if it were a normal array, you'd do document.adoptedStyleSheets.push(foo) and then nothing would happen. The browser can only react to setters/method calls. The frozen array's current API is to use the setter, but this thread is about requesting additionally adding a method call API.

@surma

This comment has been minimized.

Copy link

commented Jan 29, 2019

Fair. Thank you.

@v-python

This comment has been minimized.

Copy link

commented Mar 25, 2019

@developit 's comment on Jan 28 is key: because order is important to the list, you need to be able to insert/remove elements from specific positions, not just the front / end.

@rniwa rniwa referenced this issue Aug 14, 2019
@rniwa

This comment has been minimized.

Copy link
Author

commented Aug 14, 2019

Again, the fact people are introducing race conditions, etc... by writing a simple snippet of code in w3c/webcomponents#759 (comment) is yet another evidence that having add/remove is better than having people assign a FrozenArray.

Now I consider this issue as an absolute show stopper. I don't think we want to ever implement this feature in WebKit unless this issue is resolved.

@Rich-Harris

This comment has been minimized.

Copy link

commented Aug 14, 2019

As an aside, it already feels odd that assigning an array to document.adoptedStyleSheets results in that array being frozen. Adding add and remove methods to it feels very surprising:

const sheets = [...document.adoptedStyleSheets, sheet];
document.adoptedStyleSheets = sheets;

sheets.add(otherSheet); // huh?

Is there precedent for that elsewhere in the DOM? Having a non-reassignable object with methods for adding and removing feels much more natural, notwithstanding the ergonomic issues around prepending (though I'd expect that to be a very small minority of cases, and maybe not the thing to optimise the API for).

@fvsch

This comment has been minimized.

Copy link

commented Aug 14, 2019

Very anecdotal, but as an author seeing this in examples:

document.adoptedStyleSheets = [ ...document.adoptedStyleSheets, sheet ];

my gut reaction was that it was a useless recreation of the array and I would quickly simplify it to:

document.adoptedStyleSheets.push(sheet);

Maybe the assignment style suggests that any kind of Array manipulation (push, sort, etc.) would work? (Or maybe there are several other web APIs where you set an Array and get a FrozenArray, with similar ergonomics and restrictions, and I'm just not used to them.)

@justinfagnani

This comment has been minimized.

Copy link

commented Aug 14, 2019

The potential race condition linked to in the CSS modules thread is a problem with any of the built-in data structures like arrays. It's not unique to adoptedStyleSheets at all. I think that any sufficient alternate to adoptedStyleSheets will have possible race conditions if you read from and write to the collection with an intervening await.

@calebdwilliams

This comment has been minimized.

Copy link

commented Aug 14, 2019

Is there a reason a FrozenArray was chosen as the type as opposed to a StyleSheetList?

Could that interface be modified to support for addition, removal and reordering of stylesheet objects (even as a subclass)?

@domenic

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

StyleSheetList is like a frozen Array but with no useful reading methods (map(), filter(), etc.), and one useless method (item()). So that is why we did not choose StyleSheetList.

@calebdwilliams

This comment has been minimized.

Copy link

commented Aug 14, 2019

Right, but the question was if it could be made to be more useful …

@Rich-Harris

This comment has been minimized.

Copy link

commented Aug 14, 2019

It's not unique to adoptedStyleSheets at all

No-one is claiming that. But 'this pattern is bug-prone anyway' isn't a reason to embrace it. An API along these lines wouldn't suffer the same problem:

document.whatever.add(await import('./styles.css'));
@tabatkins

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

The potential race condition linked to in the CSS modules thread is a problem with any of the built-in data structures like arrays. It's not unique to adoptedStyleSheets at all.

More specifically, it's a problem with using await in an argument to a function (which a literal array construction is, effectively). The JS engine evaluates all preceding arguments before it pauses the function for the await, and that can cause snapshotting bugs as seen in that code.

So that is why we did not choose StyleSheetList.

Yup, StyleSheetList is just one of those shitty legacy "whoops I guess we have to act like Java" interfaces that the old DOM is plagued with. We didn't want to spread it further, since it's kinda actively hostile to web authors.

We can do both! add/remove would be sugar, in addition to allowing access to the list.

With the additional point that we probably want to make prepending as easy as appending, yeah, I'm okay with some sugar methods for append/prepend/remove, and letting array manip handle all the rest, as Emilio suggests.

@matthew-dean

This comment has been minimized.

Copy link

commented Aug 15, 2019

Yup, StyleSheetList is just one of those shitty legacy "whoops I guess we have to act like Java" interfaces that the old DOM is plagued with.

I know this is a huge undertaking and tangential aside to this discussion, but I wish there was a dedicated effort to writing a new DOM, and standardizing interfaces and methods and deprecating old ones. Like, I'd love if someone attacked the problem of "if we built a DOM from scratch in 2019, what would it look like?"

@calebdwilliams

This comment has been minimized.

Copy link

commented Aug 15, 2019

Yeah, I guess none of that actually answered the question of if StyleSheetList could be made to solve our current problem. If it exposes "no useful reading methods," what would that look like? Could that class be outfitted with the add, remove, replace, order, etc. methods that would meet the needs of CSS modules?

@rniwa

This comment has been minimized.

Copy link
Author

commented Aug 15, 2019

Yup, StyleSheetList is just one of those shitty legacy "whoops I guess we have to act like Java" interfaces that the old DOM is plagued with. We didn't want to spread it further, since it's kinda actively hostile to web authors.

I disagree with that proposition. Consistency is important.

Yeah, I guess none of that actually answered the question of if StyleSheetList could be made to solve our current problem.

Yes. In fact, I've suggested that we use StyleSheetList in numerous occasions.

@domenic

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

StyleSheetList can't be made modifiable because then it would no longer serve well for document.styleSheets, which is a read-only view onto the list of link/style-element generated style sheets.

@Jamesernator

This comment has been minimized.

Copy link

commented Aug 20, 2019

It's not possible to make an array that the browser "watches" for changes. So, if it were a normal array,

Could it work to have an exotic subclass of Array that traps all operations on it to update the document on potential changes?

e.g. If implemented in JS:

Old example
class StyleSheetArray extends Array {
  #invalidateCurrentStyles() {
    /* Recompute styles, etc, whatever browsers do when new style sheets are added */
  }

  #maybeInvalidateStyles() {
    /* Test if stuff has changed and if so invalidate current styles */
  }

  constructor() {
    super();
    return new Proxy(this, {
      get(...args) {
        const value = Reflect.get(...args);
        if (typeof value === 'function') {
          const that = this;
          return function(...args) {
            const result = Reflect.apply(value, this, args);
            this.#maybeInvalidateStyles();
            return result;
          }
        }
        return value;
      },

      set(...args) {
        const result = Reflect.set(...args); 
        this.#invalidateCurrentStyles();
        return result;
      },

      /* etc for all the other traps */
    });
  }
}

EDIT: Actually array methods are generic things so just having exotic behavior on defineProperty/deleteProperty is probably enough to observe all the changes.

e.g.:

'use strict';

function isArrayIndex(string) {
    if (typeof string !== 'string') {
        return false;
    }
    const number = Number(string);
    return Number.isSafeInteger(number)
        && String(number) === string
        && number >= 0;
}

class StyleSheetArray extends Array {
    #updateStyles() {
         /* Do usual updates when style list changes */
    }

    constructor(...args) {
        super(...args);
        return new Proxy(this, {
            defineProperty: (target, prop, descriptor) => {
                try {
                    return Reflect.defineProperty(target, prop, descriptor);
                } finally {
                    if (prop === 'length' || isArrayIndex(prop)) {
                        this.#updateStyles();
                    }
                }
            },

            deleteProperty: (target, prop) => {
                try {
                    return Reflect.deleteProperty(target, prop);
                } finally {
                    if (prop === 'length' || isArrayIndex(prop)) {
                        this.#updateStyles();
                    }
                }
            },
        });
    }
}
@calebdwilliams

This comment has been minimized.

Copy link

commented Sep 6, 2019

Should this feature be a sort of LinkedList/LinkedSet? I realize there's no true analog for those concepts in JavaScript/DOM, but implementing that based on current JS should be fairly trivial (in userland at least, I obviously can't speak for browser implementation, but I'm pretty sure most other languages have this concept built in).

That sort of object should have the following methods:

  • add (sheet: CSSStyleSheet): LinkedSet
  • remove (sheet: CSSStyleSheet): LinkedSet
  • insertAt (index: number, sheet: CSSStyleSheet): LinkedSet
  • removeFrom (index: number): LinkedSet
  • indexOf (sheet: CSSStyleSheet): number
  • includes (sheet: CSSStyleSheet): boolean
  • Symbol.iterator

Ideally I'd like to see these added to a subclass of StyleSheetList that adoptedStyleSheets could inherit from (for consistency sake). I understand that these can't be added to the base object for the reasons described in #4, but creating this sort of object should be easy enough and could then be made to respond to user actions.

Thoughts:

  • Adding a CSSStyleSheet that is already included in the LinkedSet should throw
  • Running LinkedSet.insertAt on a CSSStyleSheet that is currently in the list would reorder the list

Edit:
Overly-simplistic demo

@emilio

This comment has been minimized.

Copy link

commented Sep 7, 2019

I think a more consistent naming and API with the existing CSSOM interfaces for inserting / removing rules would be good... CSSOM interfaces have:

  • CSSStyleSheet.cssRules
  • CSSStyleSheet.insertRule()
  • CSSStyleSheet.deleteRule()

And same for CSSGroupingRule. So maybe:

  • ShadowRoot.adoptedStyleSheets (that would allow getting, not setting a frozen array... Or should it be live like cssRules? dunno)
  • ShadowRoot.removeSheet
  • ShadowRoot.insertSheet

would be the bare minimum? I agree that maybe an appendSheet would be on point, though iteration and includes you'd get for free via the FrozenArray returned by adoptedStyleSheets.

@calebdwilliams

This comment has been minimized.

Copy link

commented Sep 7, 2019

@emilio, I like the thought, but that would require additional methods on document as well. Maybe DocumentOrShadowRoot.adoptedStyleSheets.insertSheet. Maybe to preserve current functionality, adoptedStyleSheets' setter could take any iterable of CSSStyleSheets?

@heycam

This comment has been minimized.

Copy link

commented Sep 13, 2019

There is some discussion about an IDL-level feature better than FrozenArray, aimed at solving use cases like adoptedStyleSheets, here: heycam/webidl#796

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.