- Notifications
You must be signed in to change notification settings - Fork 9
Add lazy option to HOC function #9
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
Conversation
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.
| 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? |
| 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 Initially we thought about following your suggestion and just create a The 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 😝 |
| 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 |
Kind of. Like I said, it's a bit more complex than what I described. Basically the Let me show you a very simplified example (we don't actually use it this way but the idea is there). Note that the 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> ); }
Yeah we didn't really notice any performance issues, it was mostly by principle, because it's a waste to track the hundreds of
I'm ok with changing the names of the functions, but I think 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! |
| 👍 I'll publish as a minor version. Thanks! |
| @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! 🙈 |
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 😄