adopted-ember-addons / ember-light-table

Lightweight, contextual component based table for Ember
http://adopted-ember-addons.github.io/ember-light-table/
MIT License
312 stars 131 forks source link

didRender in component is called repeatedly #363

Closed Prabhakar-Poudel closed 7 years ago

Prabhakar-Poudel commented 7 years ago

I saw this strange behavior sometime back. I thought it was a problem with the code but seems different now. I have a component 'foo', inside of foo i have used ELT to render table data. There are component hooks like didRender() among others in foo component. When i run the code it seems foo's didRender hook is getting called with each cursor movement. This hook basically gets called for initial render and rerender of component. I would not say ELT is causing the problem but just for check i removed {{t.body}} from foo inside {{#light-table}}{{/light-table}}, the problem is gone and the hook is called only once. I may not have time to reproduce an example code, but I'll inform if I find something definite.

Prabhakar-Poudel commented 7 years ago

Looks like mouse move is causing the re-render consequently all the parent components in the hierarchy is calling didRender() callback.

Prabhakar-Poudel commented 7 years ago

@offirgolan Could you give a look into it?

buschtoens commented 7 years ago

Thank you for reporting this issue.

This only occurs, if one is using a fixed header ({{t.head fixed=true}}) or footer, because in that case, we wrap the table body in an {{ember-scrollable}}.

buschtoens commented 7 years ago

I'm afraid there's not much we can do about that. https://github.com/alphasights/ember-scrollable/issues/82#issuecomment-310438169

Why are you relying on the didRender hook? Maybe we can move that logic to a different place.

Prabhakar-Poudel commented 7 years ago

@buschtoens I have few things that needs to be on didRender. Thanks for looking through the problem.

buschtoens commented 7 years ago

We managed to defuse the situation with https://github.com/alphasights/ember-scrollable/pull/83. A bit that is.

After that PR is shipped, {{ember-scollable}} won't be firing didRender() for every mouse move anymore. However, it will still fire constantly while scrolling. This stems from the fact, that we have to update the virtual scrollbar. We can't work around that.

Well, we could try to update the scrollbar with jQuery, but I (and the other maintainers of {{ember-scrollable}} most likely as well) don't want to go down that path. We would be sidestepping Glimmer and its optimizations.

That's why I'm asking what exactly you're doing inside the didRender hook. Oftentimes one can restructure (read "improve") the flow of data / events or use actions in order to avoid didRender.

If this is absolutely no option, you should at least be throttling / debouncing whatever gets executed by didRender.

Prabhakar-Poudel commented 7 years ago

I am handling quite few things inside didRender, like for instance I'm adding titles to the table headers by going through each of it. Also the table is part of a larger component that houses other things beside table, and I am handling window resize events to move things inside view-port and also reset the table dimensions if required. And the fact that table data can change dynamically by setting different parameters in the parent component the set up in didRender is the only solution I could find.

If this is absolutely no option, you should at least be throttling / debouncing whatever gets executed by didRender

This should be fairly simple, and would do the job. But would still be a hack. I was wondering if there was any way to prevent didRender from bubbling when the scroll events are being tracked.

buschtoens commented 7 years ago

for instance I'm adding titles to the table headers by going through each of it

Ouch. Please don't do this. If possible, you should avoid manually fiddling around with the DOM at all costs, especially if these are elements generated by Ember.

See #368 for information on adding a title attribute to cells or table headers. Column.component is what you're looking for.

Also the table is part of a larger component that houses other things beside table, and I am handling window resize events to move things inside view-port and also reset the table dimensions if required.

Generally speaking, try to split up larger components into as many little components as possible, so that each component only has one concern.

And the fact that table data can change dynamically by setting different parameters in the parent component the set up in didRender is the only solution I could find.

I don't see why changing parameters in a parent component requires you to use didRender(). If you care, please explain it and we can try to work out a different control flow.

This should be fairly simple, and would do the job. But would still be a hack.

Not really. Depending on what you are doing, debouncing is standard practice.

I was wondering if there was any way to prevent didRender from bubbling when the scroll events are being tracked.

Nope. didRender bubbles by definition. This twiddle demonstrates that.

I can only recommend trying to avoid didRender. There's almost no case where you would need to use it.

Prabhakar-Poudel commented 7 years ago

I'll try out your suggestions in next few days, and will post update when done. Thanks again for the tips.

buschtoens commented 7 years ago

I am gonna go ahead and close this as wontfix.