fix(EventListener): do not rerender on listener prop change

[layershifter]

Fixes #1131.

[layershifter]

This sounds strange, but will work because React will call actual this.props.listener, added UTs confirm that. This behavior is covered there: https://overreacted.io/a-complete-guide-to-useeffect/#each-render-has-its-own-everything

It will be more clear when we will migrate this logic to hooks.

[kuzhelov]

might refer to another section from the same blog that warns against using shouldUpdate methods https://overreacted.io/writing-resilient-components/#dont-stop-the-data-flow-in-optimizations :) For example, now, with these changes being made, we should refrain from passing listener value any further down (the fact that we are not rendering anything helps, but there are no explicit constrains on this component in these regards) - as it will be another source of problems.

At the same time, agree that Hooks policy around when subscription happens (and, surely, around specifying update dependencies) should help in this case.

[layershifter]

Fully agree with you there, however should mention that EventListener doesn't render any children, so actually we don't stop there nothing.

And yep, hooks will be more obvious with their when 👍

[codecov]

Codecov Report

Merging #1132 into master will decrease coverage by <.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #1132      +/-   ##
==========================================
- Coverage   82.35%   82.35%   -0.01%     
==========================================
  Files         732      733       +1     
  Lines        8696     8712      +16     
  Branches     1168     1168              
==========================================
+ Hits         7162     7175      +13     
- Misses       1519     1522       +3     
  Partials       15       15

Impacted Files	Coverage Δ
...eact-component-event-listener/src/EventListener.ts	100% <100%> (+10%)	⬆️
...ent-event-listener/src/lib/shouldUpdateListener.ts	60% <60%> (ø)
...onent-event-listener/src/StackableEventListener.ts	82.75% <80%> (-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 310ea66...5224e04. Read the comment docs.

[kuzhelov]

actually, the real intent we would like to express here is that "do not update if only listener was changed". Thus, lets make this intent explicit by providing a logic like the following instead:

const shouldUpdateListener = (prevProps: EventListenerProps, nextProps: EventListenerProps) =>
  if (shallowEqual(
       { ...prevProps, listener: nextProps.listener },
       nextProps)) 
    {
       return false 
    } 
   ...

This will save us from any kind of maintainability problems that might lead to bugs that would be hard to discover - like, for example, the ones that will appear in case we would need to extend the props of this component

[layershifter]

I support this idea, however I should mention that in this case we will add a new dependency for short term... As we are going to update to React 16.8, may be we can go with existing implementation? However, I am fully agree that your proposal is more understandable...

[kuzhelov]

agree with your point - lets discuss in person and decide? or, if you feel strongly about merging, please, feel free to skip this comment :) in that case we will be able to handle this in a follow-up PR. The sole reason I am not completely sure to skip that is because shallowEqual is, essentially, a 10-lines-long func with formatting:

const shallowEqual = (first, second) => {
  // or whatever strategy we would have for handling falsy values - here is the most strict one
  if (first === undefined) { return second === undefined }
  if (first === null) { return second === null; }

  if (first === second) { return true; }

  if (Object.keys(first).length !== Object.keys(second).length) { return false; }

  const isAnyDifference = Object.keys(first).some(
    key => first[key] !== second[key]
  );

  return !isAnyDifference;
};

[layershifter]

Agreed to shallowEqual() function to this file and remove it fully when we will migrate to hooks

[kuzhelov]

Lets merge the fix with couple of things being addressed:

#1132 (comment)

Also, would suggest to provide more details in the issue - specifically, the ones that would explain what really makes it to be an issue. As an example, I might suggest to add the following insights:

• mention explicitly that event won't be handled (i.e. will be 'missed') by EventListener in case of listener prop change
• explain why it happens
• explain the fix strategy (at least in this PR's description)

This will make it much easier to review the code, as well as to understand the issue itself - otherwise it has taken me a while to follow the same steps you've already made to investigate the issue, as well as, what is more important, to figure out what makes it to be an issue.

Thank you!

[layershifter]

• mention explicitly that event won't be handled (i.e. will be 'missed') by EventListener in case of listener prop change

Actually, this was mentioned in the original issue, but I got your point about PRs description 👍

[kuzhelov]

unblocking the fix given we've synced the vision on those points I've expressed my worries about

[miroslavstastny]

I don't understand this test.
I would expect that this test was red before the changes and now it is green and helps us avoid any regression.
But is is green even before the change so what's its purpose?

[layershifter]

We agreed with @kuzhelov to remove this test as it doesn't save us from regressions