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

Create ZoomHandler #1354

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Create ZoomHandler #1354

wants to merge 7 commits into from

Conversation

Xiot
Copy link
Contributor

@Xiot Xiot commented Jun 10, 2020

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.

Comment on lines 11 to 17
const useStateWithGet = initialState => {
const [state, setState] = useState(initialState);
const ref = useRef();
ref.current = state;
const get = useCallback(() => ref.current, []);
return [state, setState, get];
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@reitlepax reitlepax Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Xiot Xiot Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
   ...
}, [])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Chris Thomas added 2 commits June 10, 2020 17:49
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 Xiot marked this pull request as draft June 10, 2020 21:50
[
enableX,
enableY,
getState,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Xiot
Copy link
Contributor Author

Xiot commented Jun 11, 2020

Any ideas on a better name for ZoomHandler ?

@Xiot Xiot marked this pull request as ready for review June 11, 2020 14:40
Storybook is an easy to use UI to enable testing out features.
This allows storybook to pick up changes from react-vis/src so that it
can be used while developing components.
@Xiot Xiot marked this pull request as draft June 11, 2020 16:12
@Xiot
Copy link
Contributor Author

Xiot commented Jun 11, 2020

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)

Chris Thomas added 2 commits June 12, 2020 23:03
Added a Window component that renders a moveable window primarily used
for visualizing a moveable viewport.
Added a storybook that showcases this.
@Xiot Xiot marked this pull request as ready for review June 13, 2020 14:12
@CLAassistant
Copy link

CLA assistant check
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants