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

eventtarget: expose "get the parent" function to support tree structure #34894

Closed
clshortfuse opened this issue Aug 23, 2020 · 12 comments
Closed
Labels
events Issues and PRs related to the events subsystem / EventEmitter. wontfix Issues that will not be fixed.

Comments

@clshortfuse
Copy link
Contributor

clshortfuse commented Aug 23, 2020

According to the EventTarget spec, an implementation may have a "get the parent" function. In a DOM structure, it's rather apparent how the tree structure structures, with parent and child nodes.

Each EventTarget object also has an associated get the parent algorithm, which takes an event event, and returns an EventTarget object. Unless specified otherwise it returns null.

https://dom.spec.whatwg.org/#ref-for-get-the-parent

While this may initially sound awkward for NodeJS, being that it doesn't DOM support at all, the ability to utilize a tree system for dispatching, capturing, and listening for events can make sense. How the tree structure is composed it outside of spec, but developers should be able to create one on their own. For example, in my personal polyfill for EventTarget and Event for NodeJS and browser environments (which I can open-source), I use a tree-structure as follows:

  • Connection Broker Service
    • HTTP Server
      • Client 1
      • Client 2
      • Client 3
    • HTTPS Server
      • Client 4
      • Client 5
      • Client 6
    • HTTP/2 Server
      • Client 7
      • Client 8
      • Client 9

Clients emits event that may be received by themselves (immediate propagation), or bubble up to the server and then the broker service. The broker service can capture events and stop immediate propagation, stop propagation, or prevent default.

Composing the path is just a "sequence", which to me means an array, so at the time of dispatch, the path is composed as follows:

let pathTarget = this;
do {
  pf.path.splice(0, 0, pathTarget);
  const targetPf = privateEventTargetFields.get(pathTarget);
  pathTarget = targetPf?.getParent?.();
} while (pathTarget);

From there, you iterate through the path in reverse during the CAPTURING_PHASE phase (items higher in the tree take preference), and forwards during the BUBBLING_PHASE. Neither EventTarget nor Event really care what's in the array, so what the user returns in getParent() is what's returned in Event.path and Event.composedPath.

Note: My private fields are based on a WeakMap for compatibility, instead of Symbol keys or private class fields.

@PoojaDurgad PoojaDurgad added the events Issues and PRs related to the events subsystem / EventEmitter. label Oct 23, 2020
@benjamingr
Copy link
Member

Hey, sorry for missing this issue.

We haven't actually implemented any of the getTheParent logic and we would rather not maintain it. We are at liberty (from the spec point of view) to do this.

We took this decision after discussing this with WHATWG folks (Domenic and Anne) and with Deno maintainers (Kitson).

I recommend we explore this with a userland library that implements EventTarget (feel free to just take lib/internal/event_target and add getTheParent).

If this use case ever becomes popular - we can/should probably upstream it.

@jasnell
Copy link
Member

jasnell commented Oct 26, 2020

+1 to that @benjamingr. My recommendation would be to close this as a wontfix

@clshortfuse
Copy link
Contributor Author

We haven't actually implemented any of the getTheParent logic and we would rather not maintain it. We are at liberty (from the spec point of view) to do this.

Actually, node isn't following spec fully. For example, I don't think capturing phase is being properly followed. Event listeners that have {capture:true} should get the the event first, which allows it to block (prevent) other event listeners from receiving the event. (Which means I can't use Node's implementation yet.) I'm okay with not having some sort of maintained method for how getTheParent works, but if we're trying to follow spec, it should call something, publically exposed or not, which by default returns null:

Each EventTarget object also has an associated get the parent algorithm, which takes an event event, and returns an EventTarget object. Unless specified otherwise it returns null.

And in dispatching events:

  1. Let parent be the result of invoking target’s get the parent with event.

https://dom.spec.whatwg.org/#dispatching-events

Having a parent, even if null, is part of the logic for capture, bubbling, and at-target phases. What is at the implementation's discretion is customized ones:

In the future we could allow custom get the parent algorithms. Let us know if this would be useful for your programs. For now, all author-created EventTargets do not participate in a tree structure.

In userland, I can then, try to make my own custom implementation (eg: WeakMap or Symbol) by just modifying whatever Node is doing (eg: calling parent = this[kGetTheParent]()). There's no reason for Node to maintain how the tree works, track tree node removals, appending, etc.

I have my own implementation that works fine, but I was thinking in process of completing the phases for the event system these couple of lines can be tacked on. In the interest of collaboration, I put the shims in a gist, so feel free to reference, copy, steal whatever to complete to spec: https://gist.github.com/clshortfuse/820fdeee78e8073a3ca07d761595ef61

No hard feeling if it doesn't get implemented. Just happy to know it's somewhere on the radar. Thanks!

@benjamingr
Copy link
Member

Actually, node isn't following spec fully.

I would love to know where - our goal is to make EventTarget fully spec complaint and we've been assisted by whatwg with this. The fact we're not 100% sure of EventTarget is why it's still experimental and not exposed to users.

In fact this approach was recommended to us by whatwg. I agree with it.

I don't think capturing phase is being properly followed. Event listeners that have {capture:true} should get the the event first, which allows it to block (prevent) other event listeners from receiving the event.

That doesn't sound right, look at how dispatch works. Propagation works across the tree (capture->target->bubble) and not on listeners on the same element:

{
  const et = new EventTarget();
  et.addEventListener('foo', () => console.log('first listener, bubble'));
  et.addEventListener('foo', () => console.log('second listener, capture'), { capture: true });
  et.dispatchEvent(new Event('foo'));
  // According to the spec (feel free to run this in Chrome too) - the above code logs:
  // first listener, bubble
  // second listener, capture
}

I'm okay with not having some sort of maintained method for how getTheParent works, but if we're trying to follow spec, it should call something, publically exposed or not, which by default returns null:

Re-reading the spec we are actually forbidden from implementing this. I think you should approach whatwg and ask for this feature in the specification. Quoting the spec, right above the quote you gave and including it:

Note: Because of the defaults stated elsewhere, the returned EventTarget's get the parent algorithm will return null, and it will have no activation behavior, legacy-pre-activation behavior, or legacy-canceled-activation behavior.

Note: In the future we could allow custom get the parent algorithms. Let us know if this would be useful for your programs. For now, all author-created EventTargets do not participate in a tree structure.

Note the spec doesn't actually expose getTheParent on the interface itself.


Again - I recommend you publish your implementation, explain where it's useful and get traction.

I swear am not doing this to "get you off our back". I am suggesting this because I believe it's a great way to experiment and gain traction. I've been involved in several projects where doing this ended up having the biggest impact. For example I and a few people wanted promises support in Node, we worked on bluebird and through it got the unhandledRejection hook and other things we believe were helpful into Node.

@clshortfuse
Copy link
Contributor Author

I'm looking at the perspective of, "What can I do with the current code to make a custom get-the-parent system?". With spec, you're supposed to dispatch the events and update the phase as events are emitted. This doesn't exist in the code. The capture should fire based on the order of .path and it's not. That means even if we were to override composedPath (in an attempt to workaround with a custom get the parent implementation), it wouldn't fire properly. For example, in Chrome:

document.addEventListener('test', (event) => console.log('regular', event.eventPhase, event.path), false)
window.addEventListener('test', (event) => console.log('capture', event.eventPhase, event.path), true)
document.dispatchEvent(new CustomEvent('test'));

will output:

capture 1 [document, Window]
regular 2 [document, Window]

But as you surely note, that's within the context of there being a tree and being a parent. At a glance (can't figure out how to test on v14), the NodeJS code has no concept of CAPTURING_PHASE or BUBBLING_PHASE. I was thinking I could override some methods or properties but it's missing the list of operations in the spec. I get why: it's not meant to fully implement a tree system. In my opinion, it follows spec if it does the operations as instructed. That means implementing a function, somewhere that returns null, as useless as it would seem, and checking against a path. Then I can override said operations. We probably have different perspective as to what it means to follow spec. Mine is probably more purist and yours is more related to whether it achieves same outcome, regardless if you follow the instructions step by step (as the comment you linked to says, "not observable").

I also read the note we both quoted as publically exposing a get-the-parent method. Ironically, my interpretation would be more liberal here and would yours more literal. I interpreted it as avoiding polluting the property list and possibly causing conflicts with future changes to the EventTarget spec. That's why I mentioned a WeakMap, or Symbol. Whatever the method is internally should be allowed to be overridden, even if not by users because of this:

Nodes, shadow roots, and documents override the get the parent algorithm.

This also brings up another point, which is that since I've made the original comment, to avoid proprietary implementations, I've reworked a couple classes of my architecture to extend Node (the W3C one). A recent use case was for resource fetching with a depth-first search algorithm (eg: CacheSystem => Redis => SQL => ExternalFetch). Any Node in the tree can fire an event and it'll propagate up the tree as in spec. It means, (theoretically but I haven't tried) I can run my service in headless Chrome and it'll work properly, but it wouldn't in NodeJS. Of course, I'd be using the document structure which Node has no active concept for. But for that even to be possible in NodeJS, some function that today returns null would later return event.target.parentNode. It also means that when trying to polyfill/shim Node in NodeJS (mouthful) there's nothing I can override in this implementation of EventTarget allowing me to do so.

Finally, let just say, I know, I know, I know, I'm bringing the absolute weirdest use cases. But I feel it's one of those things that while it seems useless in immediate context, can actually be used somewhere down the line. Again, no hard feelings if the team doesn't see it's worth the work. I'm maintaining my own shims of CustomEvent, EventTarget and Node, but would have loved to get rid of them 😆. I'd also feel more at ease with a team-maintained implementation than something I wrote myself (I literally just found a bug in it while writing this comment). I'll consider publishing though since I can see how might make more sense to get a bit more community feedback before committing to maintain something that practically nobody wants.

Thanks for the taking the time to discuss this and I appreciate all the work you're doing!

@benjamingr
Copy link
Member

So a few things regarding your example:

  • event.path is a non-standard Chrome extension. The WHATWG html specification does not define it and in most likelihood we shouldn't implement .path.
  • The only standard API we don't implement is initEvent - which makes sense because we don't have a document to create the event the old way. I think this is fine.
  • You cannot override "get the parent" today - that's what the specification requires as I pointed out above. If you found a way to do this with our EventTarget implementation and told us we would intentionally break it because we don't want to maintain such code and the specification requires user constructed EventTargets don't expose "get the parent".
  • Your example (in Chrome) fires the event on two different elements - that's "get the parent" and capture vs. bubble. That's fine, but again - we don't implement that.

I also read the note we both quoted as publically exposing a get-the-parent method. Ironically, my interpretation would be more liberal here and would yours more literal.

Opened https://github.com/whatwg/html/issues/6108

Nodes, shadow roots, and documents override the get the parent algorithm.

We do not implement any of those though :] We only implement EventTarget.

A recent use case was for resource fetching with a depth-first search algorithm

Let me make sure I understand - you create a DOM tree with (non-visual) elements that represent the caching layers and then use DOM methods to traverse it? 😮

I'm not sure that arch is super clear - but may I recommend jsdom that implements the full DOM and has a spec compliant Node and not just EventTarget?

It's maintained by DOM people and while it's not super fast - I trust it to be very correct and definitely fast enough for the use case you mention.

@clshortfuse
Copy link
Contributor Author

@benjamingr Sorry for not getting back sooner.

Yeah, so looking back. I figured asking the team to just implement a "get the parent" function would be the path of least resistance. I'd override it on my side. But when I wrote the original issue, I assumed CAPTURE_PHASE and BUBBLE_PHASE were implemented. But looking at the actual implementation, it wouldn't be as simple, there would be more work (and maintenance required).

So with that in mind, maybe the real issue should be "Support a tree structure". I would think the smarter way of doing this is adding some sort of partial implementation for Node instead of anything custom. I'm fine with closing this and I can create an issue later that is more targeted around that purpose. We can then dispute how it could and if it should be implemented. Saying "just make the method", is like putting the cart before the horse.


I think (maybe) you have misunderstood what I was asking for. You said in the whatwg comment:

A user opened an issue asking for non-null "get the parent" in Node.js's EventTarget implementation [...]

I'm not asking for this. I asking for the function to be implemented and for it to return null, so we can override it, even for the purpose of implement Node.

This is exactly what jsdom does, as you can see here:

https://github.com/jsdom/jsdom/blob/5279cfda5fe4d52f04b2eb6a801c98d81f9b55da/lib/jsdom/living/events/EventTarget-impl.js#L97-L100

They're using underscores _ to denote it's a private method not meant to the accessed publically. (I would assume NodeJS team would implement this with Symbol.) Then, in their implementation of Node, they override that private method:

https://github.com/jsdom/jsdom/blob/5279cfda5fe4d52f04b2eb6a801c98d81f9b55da/lib/jsdom/living/nodes/Node-impl.js#L140-L146

If NodeJS had an implementation similar to this jsdom that allows overriding of said method would allow us to create trees in userland (via Node or something else). I don't see anything wrong with jsdom's implementation it's entirely necessary to if you want to create an implementation of Node that implements/extends EventTarget.


Let me make sure I understand - you create a DOM tree with (non-visual) elements that represent the caching layers and then use DOM methods to traverse it? 😮

Since you asked, kinda, but not exactly. There's no document or elements. Implementation wise, document extends Element, which extends Node which extends EventTarget. I just care about a tree structures with its tree node, which is what Node does. Going as far as Element and Document aren't needed. I could create my own custom tree implementation, but why bother when Node exists and does exactly that.

It makes sense in any system with parent/child relationship. For example, if you look at the first example, architecturally speaking, all the HTTP server are siblings of each other, but there's no way to express that in code since there's no tree interface. With Node (which implements EventTarget), you could have just one event listener on the connection broker to handle all 'request' events (for middleware), instead of having to a request event listener on each and every HTTP server. Same applies to event listeners for all child connections (eg: WebSocket).

Depth-first search

Similarly, building deep-first search uses nodes in a tree. The top-down searching is an added bonus of using Node while still retaining the EventTarget bottom-up communication. The resource provider is one example, but with the connection system, I could search for a specific client connection via a tree as well. It's similar to working with HTML Elements in a detached state.

The familiarity of .parentNode and .children makes things easier especially when working with other devs, but it's just a barebones Node implementation with the purpose of composing and traversing a tree in the simplest way possible. Since there are no Element objects, there are no attributes, tag names, nor styles, and querySelector can't return anything.


Side node: Chrome's event.path seems it's just an alias of Event.composedPath(). That's part of the spec and produces the same result.


Thanks again! And thanks for the suggestions with jsdom, and all in all being patient with me. :)

@benjamingr
Copy link
Member

I... don't think that we have any interest in implementing Node to be honest - I think it's fine to implement this in userland?

I think that EventTarget only made it in because cancellation is such a fundamental and omnipresent concept we saw a lot of value in exposing AbortController and EventTarget followed as a way to do that.


I could create my own custom tree implementation, but why bother when Node exists and does exactly that.

Well, for many reasons - but for starters most trees don't have a tagName, need indexing for querySelector or so on. Node is actually quite a large interface compared to EventTarget.


They're using underscores _ to denote it's a private method not meant to the accessed publically. (I would assume NodeJS team would implement this with Symbol.) Then, in their implementation of Node, they override that private method:

Right, but they don't expose that fact publicly should indicate it's not safe to override - I've opened an issue in JSDom asking if they can guard it better. Good catch!

@benjamingr
Copy link
Member

I'm going to go ahead and close this as "wontfix" like james suggested - but thanks for the discussion.

By all means - feel free to take the code in EventTarget and publish a version of it with getTheParent on NPM (or use JSDom's)

@benjamingr benjamingr added the wontfix Issues that will not be fixed. label Oct 30, 2020
@benjamingr
Copy link
Member

@clshortfuse would you mind posting a reproducing case following the JSDOM issue template in jsdom/jsdom#3066 regarding the issue described above? (Being able to override _getTheParent).

@clshortfuse
Copy link
Contributor Author

It seems the jsdom implementation is guarding with Symbol after privatizing with underscores. You can do:

const eventTarget = new JSDOM().window.EventTarget();
const impl = Object.getOwnPropertySymbols(e).find(s => s.description === 'impl');
e[impl]._getTheParent = () => return parent;

Though, little no point to do this since you can just use the Node implementation. IMO, it's pretty well hidden anyway.

Out of curiousity, I looked at how Deno's doing it, and they do support bubbling and capturing and guard via WeakMap, similar to my implementation. They go a little further than I do with shadow DOM and slottable as well, but obviously nowhere as far as jsdom. They also support a get the parent function that will bubble up by checking if the EventTarget is what I would call "NodeLike" (has .nodeType):

https://github.com/denoland/deno/blob/9fb4931a95e551c689d4f8ed5d7304f64aafc4d0/op_crates/web/01_event.js#L433-L437

and then returning eventTarget.parentNode : https://github.com/denoland/deno/blob/9fb4931a95e551c689d4f8ed5d7304f64aafc4d0/op_crates/web/01_event.js#L423-L425

They don't implement the whole Node interface, but it seems like a good compromise for users like me to go ahead and extend it and complete the interface with the rest: (eg: children, nextSibling, etc), while retaining the bubbling and capture features of EventTarget.


Well, for many reasons - but for starters most trees don't have a tagName, need indexing for querySelector or so on. Node is actually quite a large interface compared to EventTarget.

Node actually don't that much depth. You might be thinking of Element. Node doesn't use attributes, id, or tag names. querySelector only picks up children that are Element which doesn't need to exist.

But as Deno shows, a full implementation may not be required.

@benjamingr
Copy link
Member

benjamingr commented Oct 31, 2020

Out of curiousity, I looked at how Deno's doing it

FWIW I talked with Deno maintainers when working on this feature and they are considering removing the whole getTheParent thing since it's not exposed anywhere 😅

I'll update the JSDom issue

Node actually don't that much depth. You might be thinking of Element. Node doesn't use attributes, id, or tag names. querySelector only picks up children that are Element which doesn't need to exist.

Right, if we only implement node there is no querySelector, but still a ton of stuff like nodeName, nodeValue, ownerDocument (which needs to be a document which we don't implement so guess null), textContent (which requires a rather complicated form of text processing) etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. wontfix Issues that will not be fixed.
Projects
None yet
Development

No branches or pull requests

4 participants