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

accessorFn is called in loading state with dummy skeleton rows #721

Closed
1 task done
BendixP opened this issue Oct 9, 2023 · 9 comments
Closed
1 task done

accessorFn is called in loading state with dummy skeleton rows #721

BendixP opened this issue Oct 9, 2023 · 9 comments

Comments

@BendixP
Copy link

BendixP commented Oct 9, 2023

material-react-table version

1.15.0

react & react-dom versions

18.2.0

Describe the bug and the steps to reproduce it

When the table is in a loading state and showing skeleton rows, the accessor funtion is called with the dummy row, that has undefined values for all its relevant properties. This was not the case in previous versions. It causes problems because the type of accessorFn doesn't necessarily take into account the possibility of undefined values coming in. Probably the accessorFn should not be called at all in the loading state.

Minimal, Reproducible Example - (Optional, but Recommended)

https://codesandbox.io/p/sandbox/recursing-tristan-55ph8q?file=%2Fpackage.json%3A11%2C5

Screenshots or Videos (Optional)

No response

Do you intend to try to help solve this bug with your own PR?

Maybe, I'll investigate and start debugging

Terms

  • I understand that if my bug cannot be reliably reproduced in a debuggable environment, it will probably not be fixed and this issue may even be closed.
@KevinVandy
Copy link
Owner

fixed in v2, backported to v1.15.1

@BendixP
Copy link
Author

BendixP commented Oct 16, 2023

Unfortunately there is still a problem when sorting is not disabled: https://codesandbox.io/p/sandbox/dawn-sound-64tkh7?file=%2Fsrc%2FTS.tsx%3A41%2C31 Should i open a new issue?

@dkuulis
Copy link

dkuulis commented Oct 16, 2023

@KevinVandy
Copy link
Owner

With deep accessor keys also get undefined value warnings from https://github.com/TanStack/table/blob/a1e9732e6fc3446a2ae80db72a1f2b46a5c11e46/packages/table-core/src/core/column.ts#L101

That's actually by design. In fact, we used to throw an error in this situation in earlier versions. If you have inconsistent data structures that need to be handled, I recommend using an accessorFn instead of accessorKey so that you can handle optional chaining and fallback values gracefully how you need to.

@KevinVandy
Copy link
Owner

Unfortunately there is still a problem when sorting is not disabled: https://codesandbox.io/p/sandbox/dawn-sound-64tkh7?file=%2Fsrc%2FTS.tsx%3A41%2C31 Should i open a new issue?

Just found what I needed to short-circuit in the sorting tooltip for that. Will be fixing it immediately for v2. Adding optional chaining in your accessorFn also fixes this as when blank cells are created, they are given all null values in every column, which causes problems when you call string methods. TypeScript is not warning you about this, so easy to make this error.

@BendixP
Copy link
Author

BendixP commented Oct 17, 2023

Just found what I needed to short-circuit in the sorting tooltip for that. Will be fixing it immediately for v2. Adding optional chaining in your accessorFn also fixes this as when blank cells are created, they are given all null values in every column, which causes problems when you call string methods. TypeScript is not warning you about this, so easy to make this error.

Will that fix be backported as well?

@KevinVandy
Copy link
Owner

I'll probably do one more batch of backported fixes. I planning to release v2 as stable this weekend though

@drewterry
Copy link

This is noted as fixed in the v1.15.1 changelog, but does not appear to have been backported.

@KevinVandy
Copy link
Owner

This is noted as fixed in the v1.15.1 changelog, but does not appear to have been backported.

Ok yes. If sorting is enabled, the bug is still there. But just update to the latest v2 release and it's fixed.

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

4 participants