fix(eventStack): make subscription logic to be always async

[kuzhelov]

TODO

• fix unit tests
• update changelog

---

This PR aims to fix the source of potential problems (that are very time consuming to debug) that might occur to eventStack clients - by making its subscription logic to always make async effect.

Problem description

Lets take a Popup component as an example, and its the following behavioural aspect: it should close each time when there is a click outside the popup content.

One might consider to implement this in a following way for the Popup:

// Popup.jsx
...
togglePopup() {
  const newOpen = !this.state.isOpen

  if (newOpen) { 
     // and here is the question - is there any chance this 'closePopup' logic 
     // will be executed immediately, i.e. within the same processing frame? 
     // Turns out that 'maybe'.
      eventStack.sub('click', closePopup, { target: document }) // lets specify target explicitly
  }
}

// trying hard to avoid other traps :)
closePopup = (e) => {
  this.setState({ isOpen: false })
} 

Dev's idea here is that with Popup being opened, each click event that bubbles up to document level and is not processed should cause popup to close.

The problem arises when there is another, lets say, component on the page that has subscribed to document's event via eventStack API earlier. In that case client, actually, will never see the popup in open state being rendered!

The Cause

Root cause of the problem is that eventStack is not explicit about how subscriptions are handled - those could be handled either within the same JS frame, or implicitly delayed till the next one. The reason for this difference is the following snippet of the EventTarget logic (executes on subscription):

    if (_.has(this._handlers, name)) {
       // note, effect of subscription is produced synchronously!
    }

    const handler = this._emit(name)

    // and this produces async subscription effect!
    this.target.addEventListener(name, handler)

    this._handlers[name] = handler

Solution

Solution is to make the logic consistent in terms of how subscription effect is produced - by making it always being async.

[codecov]

Codecov Report

Merging #391 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
- Coverage   91.72%   91.64%   -0.08%     
==========================================
  Files          41       41              
  Lines        1341     1341              
  Branches      197      172      -25     
==========================================
- Hits         1230     1229       -1     
- Misses        107      108       +1     
  Partials        4        4

Impacted Files	Coverage Δ
src/components/Portal/Portal.tsx	96.29% <0%> (-1.86%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37f4fd1...c59ee43. Read the comment docs.

[layershifter]

The declarative way on EventStack management makes some things more cleaner:

{this.state.open && <EventStack name="click" on={this.handleClick} target="window" />}

EventStack was cutted from SUIR core to separate package to make sure that it works fine in IE11 :)

It also was rewritten to TypeScript and published as @semantic-ui-react/event-stack: https://github.com/layershifter/event-stack

I think that makes a sense to move it to Stardust org, publish as @stardust-ui/event-stack and fix all issues there.

[kuzhelov]

@layershifter, thanks for you reply. Agree with the points mentioned, specifically:

• the declarative way on EventStack management makes some things more cleaner

  • absolutely - and, in addition to that it makes it possible to properly handle state transitioning aspects, so that all subscriptions are delayed to the next processing cycle

• It also was rewritten to TypeScript and published as @semantic-ui-react/event-stack: https://github.com/layershifter/event-stack. I think that makes a sence to move it to Stardust, publish as @stardust-ui/event-stack and fix all issues there.

  • my absolute support for this idea 👍

One thing that I would like to mention is mostly relates to the first point - while, definitely, EventStack in a form of React component will solve the issue, at the same time, given that eventStack is a library that seen to be framework agnostic, it seems reasonable to expect that even on JS level it will provide expected behavior for the most cases and will expose only the safe API. Here by 'expected' I imply the behavior one might expect if calls to eventStack would be replaced with direct calls to browser'saddEventListener API. This is the reason I see these changes still being necessary to introduce on the JS level, so it will be safe to use its functionality.

---

Also, let me mention couple of other points that we are going to introduce short term fix for, as those are quite dangerous in a sense of opened possibilities for accidental mistakes:

Case 1

eventStack.sub('focus', this._handleOutsideFocus, { target: window })
...
// nothing is unsubscribed, by default `document` is used as a target
eventStack.unsub('focus', e => this._handleOutsideFocus(e))

Case 2

eventStack.sub('focus', e => this._handleOutsideFocus(e))
...
// oops, nothing is unsubscribed
eventStack.unsub('focus', e => this._handleOutsideFocus(e))

Case 3

private _handleOutsideFocus(e) {
  ...
  // oops, I am not 'this' here (thoughts of the type's instance)
  this.handle(e)
}
...
eventStack.sub('focus', this._handleOutsideFocus)

---

Please, let me know what do you think and, most important, whether we could go for now with this set of changes, as it will unblock the possibility to introduce long-awaited functionality for the Popup component - or, in case if there are serious problems that introduced with these changes, let me know about them:) After that, as a longer-term plan, we definitely might think about moving event-stack to Stardust.

[levithomason]

Just chiming in support for @layershifter's comment as well. The plan is to make Stardust a monorepo with a "core" package, essentially the /lib. In addition, full support to @kuzhelov's point regarding the declarative React component vs the JS API: "even on JS level it will provide expected behavior for the most cases and will expose only the safe API."

I don't see Cases 1, 2, and 3 as valid problems against EventStack, however. These are true of web development in general, you must pass handlers by reference and properly use context bindings. This is how the DOM APIs work. I don't think we should try to solve some of these problems at the EventStack layer. We should not be too concerned with problems that do not originate from using Stardust.

I'm also slightly concerned about "always async". I'd rather we accomplish "always SYNC". That said, I have not reviewed the details of the code here. Just expressing what I believe would be a more optimal direction, if possible.

[kuzhelov]

let me, please, respond to the points raised:

• Just expressing what I believe would be a more optimal direction, if possible.

There is no other direction for us to implement Popup's logic reliably, so that there won't be quirky interactions with other components that are eventStack clients as well (more details in the PR description). Also won't like to wait for package being integrated into our code just to resolve this problem of Popup - we might safely start with addressing the problem given that fix is simple, and after removing this problem and being at the safe spot think about moving eventStack repo. Won't like to merge these moves into one, especially given that this feature is, apparently, not introduced there yet.

• I'm also slightly concerned about "always async". I'd rather we accomplish "always SYNC"

This won't be possible to do in theory, as some of the calls to event stack result in addEventListener calls effect of which ones is async. More detailed explanation is provided in PR description. Thus, there is only one option that is rest to us - to use async model which is, note, is the one that we will have if addEventListener calls will be used directly.

• I don't see Cases 1, 2, and 3 as valid problems against EventStack, however

I don't see them as problems against EventStack either - but the thing is that we should strive to expose safe API to the client. I cannot even stress enough how often cases 2 and 3 drives devs in confusion on our side, and even those who are pretty much aware of arrow functions - even those might think about some refactoring that will make this to be a regular function, while accidentally forgetting the context of eventStack where it is used. Needless to say that the same story would apply to its clients. But crucial thing is that, in fact, it is possible to make this API to be safe, by the following pattern:

// either of these forms will work correctly - in contrast to the original approach:
const eventSubscription = EventStackListener.create('click', () => { .. some handling logic .. })
const eventSubscription = EventStackListener.create('click', this.handle.bind(this))

// please, note that now we are SAFE to modify this function - 
// with this being arrow one, regular one everything will work
handle() { .. }

...
// and when we are done
eventSubscription.stop()

Please, note that this API is much safer to use.

---

I would strongly ask to consider to merge this or provide appealing reasons why we shouldn't do that - because this functionality is a basis for the forthcoming one of Popup. @levithomason, @layershifter , looking forward for your thoughts on that

[levithomason]

Thanks for clarification, I agree and support all these points 👍