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

callbacks do not change across renders #111

Closed
fabiosimeoni opened this issue Oct 27, 2019 · 21 comments
Closed

callbacks do not change across renders #111

fabiosimeoni opened this issue Oct 27, 2019 · 21 comments

Comments

@fabiosimeoni
Copy link

Hi there,

I might be missing something obvious, but can I access render props from getters/renderers? My dataGetter cellRenderer don't seem to update, even if they're declared as anonymous functions, and see the same initial props at each subsequent render. Intended?

f

@nihgwu
Copy link
Contributor

nihgwu commented Oct 28, 2019

the renderers/getters are called only if the data or columns changed, you can try to change the rowData to see if the getters re-called

@fabiosimeoni
Copy link
Author

thanks for the prompt reply.

it makes sense. what kind of changes to the data array trigger a refresh? I've tried with 'touching' both the array and the individual items but the dataGetter remains stale. I've had more luck with 'touching' columns though.

This might illustrate. In a component:

const random = ...generate a random something...
console.log(random)

return 
                <BaseTable width={200} height={200} data={[performance.now()]}>
                        <BaseColumn title="test" key={test} dataGetter={({rowData})=>{ console.log(random); return rowData}}/>
                </BaseTable>

the data changes at each render, yet the getter remains stuck at the first closure (continue to print the first random value)

However if I change the column at each render, e.g:

const random = ...generate a random something...
console.log(random)

return 
                <BaseTable width={200} height={200} data={["fixed"]}>
                        <BaseColumn title={`test-${performance.now()}`} key={test} dataGetter={({rowData})=>{ console.log(random); return rowData}}/>
                </BaseTable>

I do see new closures at each render, even if the data remains constant.

Apologies in advance if this is all obvious to you. I didn't expect an optimisation on something that scales along with columns - not rows - nor do I find it intuitive to 'touch' the columns to force an update. Again, I might be missing something.

thanks so far!

@nihgwu
Copy link
Contributor

nihgwu commented Oct 28, 2019

for your first example, as you said it's in a closure, so it always print the first random value, but I believe the rowData is changing, but we assume the rowData to be an object and should have an unique key like id for each item

for your second example, the dataGetter get updated every time the table get re-rendered, so it always print the latest random value, but for the first example it doesn't because BaseTable is optimized to ignore function props when comparing the columns prop

You don't need to touch the column definition to update the rows, simply update the data item, like setState({ data: [ ...data.slice(0, n - 1), {id: n, name: 'xxx'}, ...data.slice(n)] })

@fabiosimeoni
Copy link
Author

fabiosimeoni commented Oct 28, 2019

thanks. to clarify:

  • yes, the dataGetter captures the random constant in a closure, but I'm expecting the closure itself to change (and capture the latest random). I think this is clear, but just to make sure the example didn't confuse the point further,ì.

  • am using a random constant here to make the simplest simulation of a prop that changes across renders. in my setup, I do have other means to trigger re-renders and test the example, sorry I should have clarified.

that said, I'm reporting that making the data an array of objects with an 'id' prop (or using rowKey to point to a custom field} still doesn't update the getter, regardless of whether I change one of the objects in the array, or the overall cardinality of the array (by adding or removing elements).

The only thing that works for me so far is to touch the column (not necessarily the key field, a custom field would do too). But I haven't seen my dataGetter change with any of the changes to the data.

@nihgwu
Copy link
Contributor

nihgwu commented Oct 28, 2019

I think you get something wrong, update the row item means you have to change the ref of the object, so data[0].value=newValue won’t work as the row item’s ref stays the same, also you have to change the ref of the data itself or React won’t know data state changed, that’s the law of React, you don’t need to change the key, and in most cases you shouldn’t

For the closure problem, there is an open issue to track it, right now it’s recommended to pass the variable to the column definition and then access it from the render like cellRenderer=({ column }) => column.sameValue, we make it this way to avoid unnecessary re-rendering

@fabiosimeoni
Copy link
Author

okay, my example seems to have complicated matters.
in my real renders, the data array is computed brand new at each render.
similarly, the closure was a test example. In the real case I access a prop, and find it stale.
as you say, I must be doing something wrong elsewhere. thanks!

@nihgwu
Copy link
Contributor

nihgwu commented Oct 28, 2019

As I said, if you want to use data out of the rowData or column, the data will keep stale as it’s in a closure and won’t get updated because of optimization, if you do have to use outside data/state, but then in the column definition and access it from the column param of the renderer, or you can use React.Context to break the short circuit

@nihgwu
Copy link
Contributor

nihgwu commented Oct 28, 2019

See #11 FYI

@fabiosimeoni
Copy link
Author

noted! let me sum up your advice.

  • in my case I want to access the freshest version of a props.foo from a getter/renderer.
  • somehow, I can't manage to trigger a change of the getter/renderer by changing the table data (still not sure why, but let's ignore this for now).
    . you're suggesting I could put foo on the column and access it from the renderer as part of the col definition. right?

a simpler alternative, at the cost of some extra rendering work, would be to put a timestamp={Date.now()} on the column and continue to use props.foo from within the getter/renderer, which seems more 'natural' than propagating component props from component to columns.

it's working quite happily like this now. do you have a feel of how bad are the implications of touching one col this way at each render?

@nihgwu
Copy link
Contributor

nihgwu commented Oct 28, 2019

In theory your solution works as your expectation, but for me it's not nature, I'd rather read all the necessary data from the renderer's params to eliminate the closure, adding a useless timestamp to the column is somehow tricky, I think Context would be a better solution for your case?

@nihgwu
Copy link
Contributor

nihgwu commented Oct 28, 2019

BTW thanks for your patience to explain your use case, now I totally understand your problem here

@fabiosimeoni
Copy link
Author

well, am building software on top of your effort and the time you take to explain it to people. thank you rather and keep up the good work!

@nihgwu nihgwu closed this as completed Jan 17, 2020
@dufia
Copy link

dufia commented Mar 2, 2020

@nihgwu What is the recommended workaround for this issue? Context?

@nihgwu
Copy link
Contributor

nihgwu commented Mar 2, 2020

pass any state used in renderer to any of the columns, and read it from render({ column: { yourState } })

@dufia
Copy link

dufia commented Mar 2, 2020

@nihgwu we tried to avoid it, it feels hacky. Are you planning to address it in the future?

@nihgwu
Copy link
Contributor

nihgwu commented Mar 3, 2020

@dufia sure, in the next major version we won't memoize the props too aggressively

@dufia
Copy link

dufia commented Mar 3, 2020

@nihgwu thanks a lot. Sorry to bother you with this question, do you have an idea when the next major release would be?

@nihgwu
Copy link
Contributor

nihgwu commented Mar 3, 2020

Sorry I'm quite busy recently, maybe two months to go

@Selerski
Copy link

Selerski commented Jun 3, 2020

Hi @nihgwu, I'm investigating an issue similar to one @dufia had, wondering if there has been any progress with the new release? Thanks!

@sklinov
Copy link

sklinov commented Jul 7, 2021

Hi, @nihgwu ! I think I'm facing the same issue when I'm trying to pass some async stuff to cellRenderer and it looks like BaseTable is using the first closure of it.
https://codesandbox.io/s/react-base-table-async-js0to?file=/src/App.tsx

Can you help with any universal workaround for it until the next major release?

@nihgwu
Copy link
Contributor

nihgwu commented Jul 7, 2021

@sklinov https://github.com/Autodesk/react-base-table#closure-problem-in-custom-renderers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants