Remove onResize setState

[BrendanNeufeld]

This is extremely expensive. Every time the window resizes the entire app re-renders on every 'onresize' event. https://github.com/Jam3/generator-jam3/blob/master/templates/react/src/sections/App/index.js#L19

[jmckinnell]

But there are some components and sections that will need to re-render anyway because they need a new windowWidth or Height.

[njam3]

Also, if sections aren't actually using windowWidth / windowHeight, theoretically they would not be re-rendered since there would be no change to the dom.

[BrendanNeufeld]

My problem is that this encourages people to use it and consequently, potentially, unnecessary re-renders happen. I only usually have a couple listeners in an app where window dims are needed. Juniors will use this all the time and it is crazy expensive.

[BrendanNeufeld]

Essentially on every resize the section and all of the sections children will rerender. I would highly discourage this type of default behaviour. If your section needs this it should be added at the section level. they are connect route components and are in principle supposed to be isolated.

[jeremenichelli]

I don't see how this can't be accomplished with CSS at a section level. Not only that, some browsers can trigger resize event in weird situations like scroll bars showing or hiding.

I upvote this.

[BrendanNeufeld]

Although it may not cause rerenders wouldn't it cause additional evaluations as props have changed? Not sure how expensive that is. Any who I think it encourages bad practice. I would defer to css for presentational resizing and I don't think everyone knows when to use it for behaviour and when not to use it for presentation.

[BrendanNeufeld]

you are setting state on the root app component (on window resize!). if fed through props you will trigger the lifecycle for the sections and most likely the children. It will force a comparison (reconcilliation) even if the result is not inserted into the dom. or something like that.

[jeremenichelli]

It will cause a render call for sure, you can't escape that if you are setting state because it's the actual thing why setState exists, what might happen lately is that React's reconciliation algorithm compares virtual tree with actual DOM representation and sees that there's nothing to update, but all update cycle callbacks will get called, including the render function.

[njam3]

For sure it will go through a react evaluation, but this kinda seems like something that react is built to handle (evaluate updates to prevent unnecessary dom manipulation). I'm not against removing it, but I would be interesting in seeing how it affects performance (especially if no sections are using it). This has kinda been a design decision for a long time, back to bigwheel days, it would send the width and height to each section (mostly so sections don't attach their own listeners to things like resize and improperly clean them up).

Might be an idea to add it to the store so connected sections could still use it if they need to, although I'm not sure if the store is the right place for this kind of data.

[Jephuff]

A good compromise would be to debounce the resize event :)

[BrendanNeufeld]

You could have an action that dispatches on resize. https://github.com/cape-io/redux-windowsize <- just an example but I still think this is all a little dodgy. My main problem is that encourages people to use it deeply nested in their component tree forcing rerenders when it simply isn't necessary.

On Fri, Apr 7, 2017 at 5:57 PM, Nick Poisson notifications@github.com wrote:

For sure it will go through a react evaluation, but this kinda seems like something that react is built to handle (evaluate updates to prevent unnecessary dom manipulation). I'm not against removing it, but I would be interesting in seeing how it affects performance (especially if no sections are using it). This has kinda been a design decision for a long time, back to bigwheel days, it would send the width and height to each section (mostly so sections don't attach their own listeners to things like resize and improperly clean them up).

Might be an idea to add it to the store so connected sections could still use it if they need to, although I'm not sure if the store is the right place for this kind of data.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Jam3/generator-jam3/issues/276#issuecomment-292649795, or mute the thread https://github.com/notifications/unsubscribe-auth/ACm7y1NEWZ_TlcovgL0uAYdIXyBu2dQFks5rtqMvgaJpZM4M3Srl .

[njam3]

Another reason behind passing the window size down would be gaining more control over the app. If, for some reason, you wanted to render your app in only a portion of the page, simply changing this prop would handle that.

[BrendanNeufeld]

mmmm. people can add it if they need it. juniors use this too much and it forces rerenders in places they don't expect. I always remove it.

On Fri, Apr 7, 2017 at 6:07 PM, Nick Poisson notifications@github.com wrote:

Another reason behind passing the window size down would be gaining more control over the app. If, for some reason, you wanted to render your app in only a portion of the page, simply changing this prop would handle that.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Jam3/generator-jam3/issues/276#issuecomment-292652068, or mute the thread https://github.com/notifications/unsubscribe-auth/ACm7y3F2smjFLCON2r3f2WMgFrP9v_JWks5rtqWbgaJpZM4M3Srl .

[jeremenichelli]

I'm still thinking what this part of our logic is solving that can not be tackled by CSS 🤔

[cheapsteak]

Is this module on your radar react-sizeme? (Jeffrey found this btw for our current project) Can independently configure refresh rate and throttle mode on components that care to know its own size, and move that responsibility out of the root component

[njam3]

Next steps: Debouncing and researching https://github.com/wnr/element-resize-detector