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

would this be a standalone library #10

Closed
nihgwu opened this issue Jun 9, 2018 · 21 comments
Closed

would this be a standalone library #10

nihgwu opened this issue Jun 9, 2018 · 21 comments
Labels
💬 question Further information is requested

Comments

@nihgwu
Copy link
Contributor

nihgwu commented Jun 9, 2018

I created a complex Table component based on Grid of react-virtualized, I'm rewriting it now, and I'm not sure that should I use this library directly or wait for it being merged into react-virtualized as v10.

I love the simplicity of this library compare to react-virtualized, seems the original purpose of this repo is an api experimental for react-virtualized v10, I'm using some apis like onScrollbarPresenceChange from react-virtualized which are not available in this library, but I could implement them on my side if I choose to use this library. So I want to make sure what's the future of this library.

@bvaughn
Copy link
Owner

bvaughn commented Jun 9, 2018

This library will be released as react-window and will not be merged into react-virtualized. Initially, I considered releasing these API changes as version 10 of react-virtualized but decided against it because I don't intend to support as many types of components (like Collection or Masonry) and I don't want to make people choose between upgrading and losing functionality or staying on an old version.

I hope react-virtualized will continue to be maintained and released in parallel with this library, and I think there are some lessons learned here that could improve performance for react-virtualized- but that's it.

Whether you should build on top of this or react-virtualized is up to you. I won't promise to support all of the API surface for react-virtualized here (e.g. onScrollbarPresenceChange). I think the API surface for react-virtualized got too big and hard to maintain and I'd like to avoid that with this library.

Cheers!

@bvaughn bvaughn closed this as completed Jun 9, 2018
@bvaughn bvaughn added the 💬 question Further information is requested label Jun 9, 2018
@nihgwu
Copy link
Contributor Author

nihgwu commented Jun 9, 2018

Thanks for you quick response, I also agree we don't need align all the apis from react-virtualized, as I said I can implement onScrollbarPresenceChange on my side.

Then I'm going to use this library directly, thanks so much for those great libraries.

@bvaughn
Copy link
Owner

bvaughn commented Jun 9, 2018

Cool! Let me know how it turns out. 😄

@nihgwu
Copy link
Contributor Author

nihgwu commented Jun 12, 2018

It turned out that I have to use a modified version, there are several limitations for my scenario:

  1. GridItem/ListItem is over optimized, if the data set changed, the items should be re-rendered, but as Item is a pure component, which will prevent the re-render. What about leave the optimization to the user to inform them using pure component to avoid unnecessary re-render.
  2. provided keys would lead potential problems, if I insert a item in the front of the data set, it will reuse the first rendered item. What about provide an keyGetter to custom keys, that would resolve the above problem too.
  3. I need an easy way to get the estimated total rows height (I added this method to react-virtualized), but now I have to use this.gridRef._scrollingContainer.children[0].clientHeight to get it (or calculate it again on my side)
  4. I'm using the one-column Grid to implement the Table component, which is much more like the List but with horizontal scroll support. I don't want to use Grid as it adds lots of column related logic. As the above problems, I decide to modify the source code of List to satisfy my needs.

I'll let you know when the component is out, then you would have a better understanding on my concerns.

@bvaughn
Copy link
Owner

bvaughn commented Jun 12, 2018

Hey 😄 Thanks for the feedback. Here are some thoughts.

  1. GridItem/ListItem is over optimized, if the data set changed, the items should be re-rendered, but as Item is a pure component, which will prevent the re-render. What about leave the optimization to the user to inform them using pure component to avoid unnecessary re-render.

This has long been the case with react-virtualized as well, as the docs mention.

Since neither library (RV or RW) actually takes the "data set" as a prop– changes to the data set would never cause a re-render anyway, unless the item count changed, in which case both Component and PureComponent would re-render.

  1. provided keys would lead potential problems, if I insert a item in the front of the data set, it will reuse the first rendered item. What about provide an keyGetter to custom keys, that would resolve the above problem too.

This is also the same key strategy used by react-virtualized. You probably wouldn't notice the issue with RV though because react-window memoizes much more efficiently.

A custom keyGetter prop would add the overhead of an additional function call for every visible roll for every scroll event (which happens often). Items aren't expected to change position often (or at least not during a performance sensitive time– like during a scroll) so I wonder if an instance method to clear keys might be better? I don't know.

This is something I'll have to think about a bit more. 😄

  1. I need an easy way to get the estimated total rows height (I added this method to react-virtualized), but now I have to use this.gridRef._scrollingContainer.children[0].clientHeight to get it (or calculate it again on my side)

Why do you need this?

Exposing APIs like the one you describe can make it harder to maintain, refactor, and optimize the library. I learned this lesson with RV, but once something has been added to the library, it's kind of there forever. I hope to have a much higher bar for this sort of addition with react-window.

  1. I'm using the one-column Grid to implement the Table component, which is much more like the List but with horizontal scroll support. I don't want to use Grid as it adds lots of column related logic. As the above problems, I decide to modify the source code of List to satisfy my needs.

I believe you could achieve this same thing by tweaking the style parameter passed to your List item renderer to set a width other than 100%. The List should then scroll horizontally and vertically.

@nihgwu
Copy link
Contributor Author

nihgwu commented Jun 12, 2018

@bvaughn for the first one, I think you missed my description? I'm talking about the GridItem not the Grid itself, you can see there are no extra props being passed into it, and it's a pure component, so I just can't make the Item re-render, like reorder the data, unless I reset the cached styles

for the second one, I do encounter with some issues because of that with CRUD, if we just remove the GridItem and leave the optimization to the user, we can pass the rowData.id or something else as the key, then there won't be extra function calls

for the third one, I explained the scenario why I need that api in RV, as I want to implement auto height feature with CRUD and expanding, I don't wont to recalculate the total height as it's already been done in the library

the last one, in that way I still can't get scrollLeft from onScroll, I need it to implement frozen columns feature

@bvaughn
Copy link
Owner

bvaughn commented Jun 12, 2018

@bvaughn for the first one, I think you missed my description? I'm talking about the GridItem not the Grid itself, you can see there are no extra props being passed into it, and it's a pure component, so I just can't make the Item re-render, like reorder the data, unless I reset the cached styles

Whoops, I did misread your first one. I'll have to give that one some more thought too.

for the third one, I explained the scenario why I need that api in RV, as I want to implement auto height feature with CRUD and expanding, I don't wont to recalculate the total height as it's already been done in the library

Okay. I don't remember every feature request react-virtualized gets. 😄

the last one, in that way I still can't get scrollLeft from onScroll, I need it to implement frozen columns feature

Why would you need scrollLeft for a one-column list/grid? Are you trying to add some sort of scroll-syncing feature?

@nihgwu
Copy link
Contributor Author

nihgwu commented Jun 12, 2018

Why would you need scrollLeft for a one-column list/grid? Are you trying to add some sort of scroll-syncing feature?

correct, for example, I have a header and a body(Grid) to compose the table, when I scroll the body horizontally, I need to sync the scrollLeft of header too

@bvaughn
Copy link
Owner

bvaughn commented Jun 12, 2018

Gotcha. This is useful input. I'll give it some thought, at least. No promise to make any changes yet. 😄

@nihgwu
Copy link
Contributor Author

nihgwu commented Jun 12, 2018

No hurry, I'm working on a modified version to meet my needs now, hope I can finish the migration soon, here is a peek 😄
frozen-rows

@bvaughn
Copy link
Owner

bvaughn commented Jun 12, 2018

Cool, cool. I look forward to seeing the changes you make. Maybe some of them can make their way back upstream.

I'd also be curious to know about how the changes impact performance (if at all). 😄 The new React.unstable_Profiler component could offer some insight here, perhaps. I've been using it to compare react-virtualized 9 to a few of the alpha builds.

PS. The demo looks cool! 😄

@bvaughn
Copy link
Owner

bvaughn commented Jun 16, 2018

I've given some more thought to your "pure" component comment. I've decided to treat the children prop as a component rather than just a function, and to remove the default PureComponent wrapper I was using before.

This means that function components won't be memoized by default, but that's probably okay. If you want to avoid re-renders for complex items, you can extend PureComponent or implement shouldComponentUpdate yourself.

This change has been published in the new alpha (2).

I have not yet decided about the key thing you mentioned. The short answer is that this will "work" the way you want now, by default– since there is no PureComponent wrapper being injected from my side. But if you want to use memoization and be able to move items around, you'll still have this problem.

You could work around it by implementing your own shouldComponentUpdate method, but that's kind of a hack and I don't really want to encourage custom sCU implementations.

@bvaughn
Copy link
Owner

bvaughn commented Jun 16, 2018

Okay, after some consideration– I've decided to add a new itemKey prop as well, to enable custom keys to be provided. This will be an optional property. I've included this in alpha 3.

@nihgwu
Copy link
Contributor Author

nihgwu commented Jun 16, 2018

itemKey is good, i'm using dataKey for my table component
UPDATE: you are treating itemKey as a function, but I use it as a string

@nihgwu
Copy link
Contributor Author

nihgwu commented Jun 16, 2018

for the item, what's the different between pass a children and itemRenderer, I changed the children into itemRenderer in my version

BTW, is there any performance concern to memoize function(param1, param2) over function({param1, param2}), I think if we use the later form directly there is no need to memoize? I removed memoize as it's always broken after HMR

@bvaughn
Copy link
Owner

bvaughn commented Jun 16, 2018

for the item, what's the different between pass a children and itemRenderer, I changed the children into itemRenderer in my version

Subjective preference. I like using children better because I think it's cleaner and reads better for the function component use case.

BTW, is there any performance concern to memoize function(param1, param2) over function({param1, param2}), I think if we use the later form directly there is no need to memoize? I removed memoize as it's always broken after HMR

I don't really understand what you're asking, sorry.

Typical memoization won't work if you create a new wrapper object each time you call the function.

@nihgwu
Copy link
Contributor Author

nihgwu commented Jun 16, 2018

I don't really understand what you're asking, sorry.

image
I'm referring to this block, you simply memoize and proxy function(param1, param2) to function({param1, param2}), I'm wondering if it's really necessary as the params change all the time

@bvaughn
Copy link
Owner

bvaughn commented Jun 16, 2018

This statement still does not make sense to me:

is there any performance concern to memoize function(param1, param2) over function({param1, param2}), I think if we use the later form directly there is no need to memoize?

The onScroll callback should only be called if one of its properties change. Since it uses named parameters (aka an inline object), its properties will always appear to "change" from the POV of a memoization library.

So I use a memoization wrapper with regular, non-named parameters– (which is fine, because I'm not worried about getting their order messed up locally)– to control when I call the external prop– (which uses named parameters, for users convenience).

This is nice because it frees me up from having to explicitly track and compare each individual parameter before calling the function.

@nihgwu
Copy link
Contributor Author

nihgwu commented Jun 16, 2018

Since it uses named parameters (aka an inline object), its properties will always appear to "change" from the POV of a memoization library.

That’s what I expected, thanks.
Now I get it, as onScroll is not called directly from onScroll event, so there are chances the parameters stay the same, I thought the onScroll callback would be invoked with different parameters all the time

@sandys
Copy link

sandys commented Oct 15, 2018

@nihgwu - would you consider releasing your table component as open source ? There's a lot we can learn from it as an illustrative example in using react-window.

Here's our attempt on the react-virtualized side :
https://github.com/RedCarpetUp/rv-rich-table

@nihgwu
Copy link
Contributor Author

nihgwu commented Mar 26, 2019

Here it is, BaseTable with frozen rows and columns support and other features, but based on react-virtualized right now, I planed to migrate to use react-window in the next version
I didn't expect it would take so long to open source it (I'm a bit busy with other stuffs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants