-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
Comments
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. |
+1 to that @benjamingr. My recommendation would be to close this as a wontfix |
Actually, node isn't following spec fully. For example, I don't think capturing phase is being properly followed. Event listeners that have
And in dispatching events:
https://dom.spec.whatwg.org/#dispatching-events Having a parent, even if
In userland, I can then, try to make my own custom implementation (eg: 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! |
I would love to know where - our goal is to make In fact this approach was recommended to us by whatwg. I agree with it.
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
}
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 the spec doesn't actually expose 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 |
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 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:
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 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
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 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 Thanks for the taking the time to discuss this and I appreciate all the work you're doing! |
So a few things regarding your example:
Opened https://github.com/whatwg/html/issues/6108
We do not implement any of those though :] We only implement
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 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. |
@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 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 I think (maybe) you have misunderstood what I was asking for. You said in the whatwg comment:
I'm not asking for this. I asking for the function to be implemented and for it to return This is exactly what They're using underscores If NodeJS had an implementation similar to this
Since you asked, kinda, but not exactly. There's no document or elements. Implementation wise, 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 Similarly, building deep-first search uses nodes in a tree. The top-down searching is an added bonus of using The familiarity of Side node: Chrome's Thanks again! And thanks for the suggestions with |
I... don't think that we have any interest in implementing 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
Well, for many reasons - but for starters most trees don't have a
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! |
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 |
@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). |
It seems the 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 Out of curiousity, I looked at how Deno's doing it, and they do support bubbling and capturing and guard via and then returning They don't implement the whole
But as Deno shows, a full implementation may not be required. |
FWIW I talked with Deno maintainers when working on this feature and they are considering removing the whole I'll update the JSDom issue
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 |
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.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
andEvent
for NodeJS and browser environments (which I can open-source), I use a tree-structure as follows: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:
From there, you iterate through the path in reverse during the
CAPTURING_PHASE
phase (items higher in the tree take preference), and forwards during theBUBBLING_PHASE
. NeitherEventTarget
norEvent
really care what's in the array, so what the user returns ingetParent()
is what's returned inEvent.path
andEvent.composedPath
.Note: My private fields are based on a
WeakMap
for compatibility, instead ofSymbol
keys or private class fields.The text was updated successfully, but these errors were encountered: