Skip to content

Conversation

@braposo
Copy link
Contributor

@braposo braposo commented Jan 14, 2017

As explained in the README, there can be situations where we need to defer the creation of the watcher until a given condition is met. In our case the problem is that we're using the HOC to enhance a component that is used in many places but only want to create the watcher for some of them.

The proposed change is to just add an extra argument to the HOC and avoid automatically creating the watcher if it's set to true, meaning it's in "lazy mode".

I also made a couple of other improvements (move react and react-dom to peerDependencies and add yarn lockfile) and updated the syntax for clarification (semi-colon on class properties and changed to nextProps on componentWillReceiveProps).

Let me know what you think of this 😄

This avoid creating a watcher to every component automatically, which is specially useful if applying the HoC to a component that is used in multiple situations but only want to watch the behaviour in some of them.
As far as I can understand, there’s no reason why react and react-dom need to be included as normal dependencies. They should be listed as peer dependencies so each user manages them as they wish.
@stutrek
Copy link
Owner

stutrek commented Jan 16, 2017

I like your code, and the documentation is great, but I'm not sure if the added complexity is in the right place.

Why not use the unwrapped version of your component in addition to the wrapped version?

Is a component going from unwatched to watched a common use case?

@braposo
Copy link
Contributor Author

braposo commented Jan 17, 2017

hey @stutrek, thanks for the comment! Let me try to explain my problem so it's (hopefully) easier to understand where this would be applicable.

We're currently developing a grid/layout library that has a utility component called <Block>. This component is used as the base component of the grid and allows us to define things like breakpoints where it's visible or the vertical alignment of its children. Being an utility component, we also use it for more advance things like detecting the window size on resize so we can trigger some actions if it enters/leave a given breakpoint or, and this is where scrollmonitor enters, reacting when it enters/leaves the viewport, among other things.

Initially we thought about following your suggestion and just create a <WatchableBlock> component that we could independently use. The problem is that, in order to have this advanced features, we implemented the <Block> component as an abstraction for the <Watcher> component that basically is the single source of truth (holds the state) for the resize and scroll listeners.

The <Watcher> component is the one that is actually wrapped by the Watch() HOC and because it also listens to other events (like resizing) we didn't want to create the listeners for all instances of the component. It's a little bit more complex than this and it's not easy to explain without showing the code, but I hope it's clearer now.

To sum up, it's easy to just have an enhanced component when the only thing that you want to track is the scroll behaviour, but if you want that enhanced component to track more events you'll need a way to avoid creating the listeners all the time.

As you noticed, this change is completely transparent to previous usage of the library, meaning nothing will break and people can still use it as they always have been. I just thought this could be helpful for a more complex usage of the library and also give more flexibility to facilitate the adoption by other library authors.

I totally understand your concerns about adding complexity, but these changes have virtually no cognitive load to the user as they can simply ignore them. This is just for the cases where you end up thinking "I wish this had a way to explicitly control the creation/destruction of the listeners", which I know is not that often but it happens sometimes 😝

@stutrek
Copy link
Owner

stutrek commented Jan 17, 2017

I see, an interesting problem. Is the part of your Block that does the resize watching a HOC or a regular one? I'd like to see that implemented in a library.

Am I right in thinking that you're trying to know if items are in the viewport only at certain window sizes? My strategy was to make it fast enough that you don't need to care whether or not it's watching, but the state properties still update and can cause a rerender, but this is absolutely going to become a more common use case as UIs get responsive.

I think this functionality could be useful outside of lazy components too. If initWatcher and destoryWatcher are named something like pause and resume and don't check lazy it might be more useful. Also, why isn't lazy a prop?

@braposo
Copy link
Contributor Author

braposo commented Jan 18, 2017

I see, an interesting problem. Is the part of your Block that does the resize watching a HOC or a regular one? I'd like to see that implemented in a library.

Kind of. Like I said, it's a bit more complex than what I described. Basically the <Watcher> is implemented as a function as a child component, which is conceptually similar to the HOC but is more appropriate for what we need.

Let me show you a very simplified example (we don't actually use it this way but the idea is there). Note that the <Watcher> component is the one being enhanced by the Watch() HOC.

class Block extends React.Component { ... return ( <Watcher trackBreakpoints={this.props.trackBreakpoints} trackViewport={this.props.trackViewport} > { ({currentBreakpoints, isInViewport}) => { if (!this.shouldRender(currentBreakpoints)) { return null; } if (isInViewport) { // do something (change styles, trigger animation, etc) } return ( <div> {this.props.children} </div> ); } } </Watcher> ); }

Am I right in thinking that you're trying to know if items are in the viewport only at certain window sizes? My strategy was to make it fast enough that you don't need to care whether or not it's watching, but the state properties still update and can cause a rerender, but this is absolutely going to become a more common use case as UIs get responsive.

Yeah we didn't really notice any performance issues, it was mostly by principle, because it's a waste to track the hundreds of <Block> components we have to only actually use the scroll monitor in a few cases. For now we just need to trigger a few animations on scroll, but want to implement a solution that is abstract enough to use in the future if we need to do something with the scrollmonitor state/callbacks.

I think this functionality could be useful outside of lazy components too. If initWatcher and destoryWatcher are named something like pause and resume and don't check lazy it might be more useful. Also, why isn't lazy a prop?

I'm ok with changing the names of the functions, but I think start and stop would make more sense if the watcher is running in "lazy mode", meaning it's stopped by default. Lazy can absolutely be a prop, when I first thought of this it was more of a initialisation setting so it made sense to be in the HOC call but I realise it might be easier to understand if it's just a prop you can send. Probably with a better name as well like autoStart, because "lazy" can be too hard to understand. autoStart would be true by default but we could set it to false to have the same behaviour as the lazy argument I suggested. Like that idea 👍

Actually, I think it simplifies a lot of the logic and you don't even need to call the start/stop functions (although they could be useful for the reasons I mentioned before). Something like this would work out of the box:

<WatchedComponent autoStart={this.props.trackViewport ? true : false}>

I'm going to make this change anyway because I think that's an improvement regardless of the final decision on this. Let me know what you think!

@stutrek
Copy link
Owner

stutrek commented Jan 19, 2017

👍 I'll publish as a minor version. Thanks!

@stutrek stutrek merged commit 67110a8 into stutrek:master Jan 19, 2017
@braposo braposo deleted the add-lazy branch January 23, 2017 01:24
@braposo
Copy link
Contributor Author

braposo commented Jan 23, 2017

@stutrek sorry for the delay but thanks a lot for this! I noticed however that I forgot to change the function names to use the correct ones so I'll do a separate PR to fix it, sorry about that! 🙈

@braposo braposo mentioned this pull request Jan 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants