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

ensure columnPinning.[property] is not accessed unsafely #5519

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rustysys-dev
Copy link

Had issues with the qwik examples etc... things just didn't really work at all, and it seems like unnecessary processing is being done in the lib..

In anycase, I decided to build my own table using the core lib directly for qwik.. however I am running into a bug, where if you provide an empty state to start off with the entire app crashes on trying to read the left property on columnPinning as shown:

image

I notice that most (not all) other functions in the code which do something similar are not accessing it safely with columnPinning?.[property]

@KevinVandy
Copy link
Member

Maybe it would make more sense for us to make sure all of the typescript types enforce the full object is always present?

@rustysys-dev
Copy link
Author

@KevinVandy I think that would satisfy the requirement as well, but I suppose I should ask, is column pinning really required to be defined? If it must be defined for other features etc to work then enforcement via typescript is definitely best.

If not then a thought in the back of the head is that it would cause unnecessary development required when using the core lib.

@KevinVandy
Copy link
Member

@KevinVandy I think that would satisfy the requirement as well, but I suppose I should ask, is column pinning really required to be defined? If it must be defined for other features etc to work then enforcement via typescript is definitely best.

If not then a thought in the back of the head is that it would cause unnecessary development required when using the core lib.

Yes, it is assumed that all of the getInitialState calls for each feature have been called successfully.

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

Successfully merging this pull request may close these issues.

2 participants