Create ZoomHandler

[Xiot]

Due to the way that the Highlight component handles the mouse events,
it is pretty much required that it is one of the last children of the XYPlot.
Since it renders a transparent rect over the entire XYPlot it currently disables
any mouse events for controls lower in the dom tree.
If the Highlight component is moved higher in the dom tree, then the onMouseLeave
event will fire when a lower component is hovered over.

The ZoomHandler delegates the handling of the mouse events to the XYPlot itself.
The XYPlot now holds a collection of Event objects that will be passed to the children,
where they can subscribe to the ones that they are interested in.
When the appropriate Event in the XYPlot is fired, it will execute the callbacks for all listeners.
This approach does not cause the XYPlot to re-render, and only the listeners that perform an
operation in their callback will be re-rendered.

Also created a useStateWithGet that wraps a useState call and also provides a memoized getState method.
Since the getState method is only created once for the component, it will not trigger re-renders when
listed in the dependencies of useEffect / useCallback. This drastically cuts down the number of
event handlers that are subscribed / unsubscribed.

[Xiot]

I will move this to a shared location, however I left it here for now to give it some better visibility.

This acts just like useState however it returns a third parameter which is a function that returns the value.
The function is memoized so there will only be one instance per component, however when called, it will return the current state.

This makes it ideal when the handlers, such as useEffect and useCallback need the current value, however it doesn't change the identity of handler.

Using the getState in the useCallbacks below drastically reduces the number of subscribe/unsubscribe calls, as well as the number of re-renders. Without this, the callback for onMouseUp would be invalidated on every mouse move (while dragging)

[reitlepax]

I am not very comfortable with this, if I understand correctly it looks like a hack to get around the dependencies of useCallback and useEffect and this could lead to a bug.

I am not talking about this specific case because I am not sure, but it could lead to situations where the state value is changed, but since getState is used as dependency for a useEffect , it won't be called with the new value

[Xiot]

Correct, this is an approach to get around the dependencies and it could be abused which may cause bugs.

The main bug that I can see arising only really applies to useMemo.
If the getState is passed into the dependencies of useMemo a change of state will not cause the useMemo handler to re-fire, and therefore the result would be based off of the previous value.

With useCallback and useEffect, you won't really have these problems since they would always use the latest values, and their execution is not dependant on the render function.

One other way to accomplish this would be

const onMouseUp = useCallback(() => {
  setState(state => {
     if (size > 5) {   // shorted for brevity
        onZoom(state.bounds);
    }
   setState(DEFAULT_STATE);
}, [])

This example here, I find is much worse as the function passed to setState is no longer pure.
Here is an issue discussing other ways around it as well. facebook/react#14543

[Xiot]

Another, more standard way of doing this, would be as follows:

const [state, setState] = useState(DEFAULT_STATE);
const stateRef = useRef(state);

const onMouseUp = useCallback(() => {
   const state = stateRef.current;
   ...
}, [])

In the end this is the same as the custom hook, however this is more explicit.
And as such, it could be commented in-line to better explain why the functionality is like that.

This would also eliminate the temptation to use this in places where it would cause bugs.

[reitlepax]

With useCallback and useEffect, you won't really have these problems since they would always use the latest values, and their execution is not dependant on the render function.

I am not sure I follow. If I have a useEffectdeclaring getState as a dependency, I expect useEffect won't be re-run even if the state has changed.

What do you mean by "execution not dependent on the render function" ?

[reitlepax]

Issues can arise in situations like this though

Yes I agree, that was the case I meant in my first comment, and why I am not fond of giving a tool like this helper which can bring bugs but that's just my opinion. What K like about hooks is that if you follow the eslint rules and stick to them you should can this kind of bugs. In the meantime, I hear that performance issues can arise with too many re-renders

This mainly refers to useCallback

Yes again I agree this applies to useCallback but I was more on the fence for this statement about useEffect

[reitlepax]

If this helper brings a lot of value I won't fight over it even if I have concerns

I really like what you suggested in this comment: #1354 (comment)

[Xiot]

Ya. I misspoke putting useEffect in that sentence. The point that I was trying to make with putting in there was that the value in the state doesn't necessarily affect the lifecycle of the effect.

How do you feel about the explicit approach?

const [state, setState] = useState(DEFAULT_STATE);
const stateRef = useRef(state);

const onMouseUp = useCallback(() => {
   const state = stateRef.current;
   ...
}, [])

[reitlepax]

That way it is more self-explanatory and as you said we won't be tempted to use it in places it can cause bugs

[Xiot]

OK. I'll change it to use the explicit version.
I'll also make sure to comment it so that the reader knows why the state is being stored in the ref

[reitlepax]

I'm not sure but I think iif you break the state in several useState, you could have
const [bushing, setBrushing] = useState(false) const [position, setPosition] = useState(null) const [bounds, setBounds] = useState(null)
Then you can brushing as a dependency instead of getState, which means you declare the real dependencies and you don't need to "cheat" about the deps with getState

Could that solve your issue about the number of subscribe/unsubscribe call and the number of re-renders ?

[Xiot]

The biggest culprit is the fact that onMouseUp needs the bounds which changes every-time the mouse moves which brushing.
Using a separate useState for brushing wouldn't be too bad since that only changes onMouseDown / onMouseUp.

I'm also wary about splitting up the useState calls because then I would need to make 3 calls to state setter functions in both onMouseUp and onMouseDown. I would assume that React would batch these calls and only cause 1 re-render, but I'm not 100% sure. I know that in React Native calling setState with a function actually executes that asynchronously so I lose the ability to reason when the state will be changed.

[reitlepax]

Indeed I did not have in mind the need for onMouse handlers to update the whole state.

That's too bad I thought splitting bounds could solve part of the issue, but if I recall react does not necessarily batch multiple setState calls (I believe they often are batched react-dom, but there is no guarantee it's 100% of the time so we can't rely on it).

Since the goal is to improve perf, splitting the state wouldn't be an improvement so let's drop that suggestion

[Xiot]

Any ideas on a better name for ZoomHandler ?

[reitlepax]

I am not the greatest to find names, I think ZoomHandler is fine!

[Xiot]

Converting this back to a draft as I want to change up the api a bit.
The zoom handler doesn't actually zoom anything... it just handles the selection
I'm going to change it to just handle the selection, and I'll make a separate component for the draggable window that Highlight has (although this may be in a separate PR)

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Chris Thomas seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}