ready.js could deduplicate readystate listener by target

[klebba]

the DOMReady util ready currently adds new event listeners every time it is invoked; instead it could create a mapping of callbacks keyed to targets and avoid making duplicate listeners

ref: https://github.com/Netflix/x-element/blob/82283f53d36f01ee91640818c76144cf99ecb255/etc/ready.js

[theengineear]

@klebba — what would you think about just not having /etc in this repo anymore? It feels different-enough from the main goal of x-element that I'm not certain it even belongs in here.

[klebba]

@theengineear I think ready.js will eventually become core browser behavior (see: whatwg/html#127 as well as whatwg/html#1936) but it's taking some time to get traction there. I often use ready.js when integrating third party libraries with x-element, so IMO it would be frustrating to lose it, but I can see the argument for that

[theengineear]

I'm ok to leave it in if the intention is that it will be replaced with platform-level behavior. Perhaps one action item would be to document that fact in the code.

[theengineear]

@klebba — Just looking to follow-up on some issues in this repo. Do you have an example of when we would use ready without passing document in? I ask because I'm tempted to rewrite the file as follows to lock down how it's meant to be used:

// This functionality can be replaced by the "document.loaded" which will be
//  introduced in https://github.com/whatwg/html/pull/1936.
const documentLoadedPromise = new Promise(resolve => {
  if (document.readyState === 'complete') {
    resolve();
  } else {
    const handle = () => {
      if (document.readyState === 'complete') {
        document.removeEventListener('readystatechange', handle);
        resolve();
      }
    };
    document.addEventListener('readystatechange', handle);
  }
});
const ready = () => documentLoadedPromise;

export default ready;

[klebba]

I think on principal we should not assume the event target here, but I don't feel super strongly about this. If you prefer we could include something like this in the file:

[...]

const documentReady = ready(document);
export documentReady;

[theengineear]

My goal was to make the functionality in ready.js completely replaced by the forthcoming whatwg/html#1936 functionality. Then, the road to deletion is really clear. Additionally, by limiting the functionality, it obviates the need for deduplicating the listener (since there'd only ever be one).

Did you have a use-case for ever passing in anything other than document?

[klebba]

I remember needing to pass document before in a situation where I'm awaiting readiness of the contents of an <iframe> — however I haven't been able to find any useful sample code yet. Modifying the interface here would be a breaking change, and while I know we haven't declared the library interface is stable yet, I would still recommend deferring until browsers start landing the proposed new document.loaded functionality in canary builds.

[theengineear]

Modifying the interface here would be a breaking change.

I don't think this would be an issue. It's going from target => { /* do the thing */ } to () => { /* do the thing */ }.

I would still recommend deferring.

So by "deferring" here, this means "deduplicate readystate listener by target"? I was trying to deduplicate by simplifying, but I can build it out by maintaining a mapping instead.

[klebba]

Oh yeah, you're totally right — good call. Nothing would break. What about something like:

const ready = (target = document) => documentLoadedPromise;

Also to clarify, what I meant by my deferring comment is "I recommend we wait before making changes" — mostly because the browser implementations aren't yet settled. Ultimately I'm in favor of making your recommended simplification to address the initial impetus for the ticket. You are right that it's minor

[theengineear]

☝️ — Sure, I can make it the default. If a user passes in something other than document, shouldn't we do the right thing though? I'll put up a PR to discuss.