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

ready.js could deduplicate readystate listener by target #65

Open
klebba opened this issue Nov 23, 2020 · 10 comments
Open

ready.js could deduplicate readystate listener by target #65

klebba opened this issue Nov 23, 2020 · 10 comments

Comments

@klebba
Copy link
Collaborator

klebba commented Nov 23, 2020

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
Copy link
Collaborator

@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
Copy link
Collaborator Author

klebba commented Dec 18, 2020

@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
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Collaborator Author

klebba commented Apr 12, 2022

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
Copy link
Collaborator

theengineear commented Apr 13, 2022

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
Copy link
Collaborator Author

klebba commented Apr 13, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator Author

klebba commented Apr 15, 2022

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
Copy link
Collaborator

☝️ — 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.

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

No branches or pull requests

2 participants