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

Small updates to the TypeScript types #65

Merged
merged 3 commits into from
Nov 17, 2022
Merged

Conversation

nvie
Copy link
Contributor

@nvie nvie commented Nov 15, 2022

This doesn't change anything about the implementation. It just changes the types to match the current reality a little bit more strictly.

nvie added 2 commits November 15, 2022 16:39
This already is the case and works, just updates the types to reflect that reality.
@nvie nvie changed the title Allow childrenAccessor function to return null Small updates to the TypeScript types Nov 15, 2022
Copy link
Member

@jameskerr jameskerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@jameskerr
Copy link
Member

Looks like the tsc script failed due to the readonly addition. If you remove that, I think the other change is good for this PR.

@nvie
Copy link
Contributor Author

nvie commented Nov 16, 2022

Hey @jameskerr! Thanks for the feedback, I just noticed the tests were not passing, so I made a few more internal annotations to fix those. I now tested these locally by running the build script, and they now pass the tsc checks.

The benefit of expressing that the data you pass won't be mutated by marking it readonly on the outside is that you can feed both mutable and readonly arrays to the Tree component this way.

In my use case, I found myself writing <Tree data={[...myReadonlyArray]} /> instead of just <Tree data={myReadonlyArray} />.

nvie added a commit to liveblocks/liveblocks that referenced this pull request Nov 16, 2022
This allows us to remove a few type casts that were unnecessary, but
we'll have to put in a couple more, because React Arborist needs to
update their types.

See brimdata/react-arborist#65
@jameskerr
Copy link
Member

Great! That makes sense to me.

@jameskerr jameskerr merged commit cc941da into brimdata:main Nov 17, 2022
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