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

fix(virtual-core): add bigint to Key to align with @types/react implementation #814

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

huv1k
Copy link
Contributor

@huv1k huv1k commented Aug 28, 2024

This PR enhances Key type with bigint to align with React.Key. This was updated in DefinitelyTyped/DefinitelyTyped#66723. This will enable us to use React.Key as type for where we require Key as type.

@SaeWooKKang
Copy link
Contributor

If changed like that, it might be more convenient when using React.

However, Should the higher-level element 'core' depend on the lower-level element 'react'?
How about using type casting in React?

const alsoHuge = BigInt(9007199254740991);

alsoHuge.toString();

Copy link

nx-cloud bot commented Aug 28, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 892a4d5. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

Copy link

pkg-pr-new bot commented Aug 28, 2024

commit: 892a4d5

@tanstack/angular-virtual

pnpm add https://pkg.pr.new/@tanstack/angular-virtual@814

@tanstack/lit-virtual

pnpm add https://pkg.pr.new/@tanstack/lit-virtual@814

@tanstack/react-virtual

pnpm add https://pkg.pr.new/@tanstack/react-virtual@814

@tanstack/solid-virtual

pnpm add https://pkg.pr.new/@tanstack/solid-virtual@814

@tanstack/svelte-virtual

pnpm add https://pkg.pr.new/@tanstack/svelte-virtual@814

@tanstack/virtual-core

pnpm add https://pkg.pr.new/@tanstack/virtual-core@814

@tanstack/vue-virtual

pnpm add https://pkg.pr.new/@tanstack/vue-virtual@814

Open in Stackblitz

More templates

@piecyk
Copy link
Collaborator

piecyk commented Aug 28, 2024

However, Should the higher-level element 'core' depend on the lower-level element 'react'?

@SaeWooKKang allowing bigint as key is not really connected to react

@huv1k
Copy link
Contributor Author

huv1k commented Aug 28, 2024

Yes, the problem is when you use Key from react, because it has a different type and there is an error, which shouldn't be the case. Is there anything more needed?

@SaeWooKKang
Copy link
Contributor

@SaeWooKKang allowing bigint as key is not really connected to react

You're right, there isn't a direct connection. However, my comment was about whether it's appropriate to add a type to the key specifically for the purpose of using it with React.

Yes, the problem is when you use Key from react, because it has a different type and there is an error, which shouldn't be the case. Is there anything more needed?

Is there no need to modify the documentation for the type?

@piecyk
Copy link
Collaborator

piecyk commented Aug 29, 2024

However, my comment was about whether it's appropriate to add a type to the key specifically for the purpose of using it with React.

Yep, thanks for the input. I think we are fine here, as bigint is overall an primitive not a react thing.

Is there no need to modify the documentation for the type?

good catch, @huv1k can you update docs at https://github.com/TanStack/virtual/blob/main/docs/api/virtual-item.md

@huv1k
Copy link
Contributor Author

huv1k commented Aug 29, 2024

However, my comment was about whether it's appropriate to add a type to the key specifically for the purpose of using it with React.

Yep, thanks for the input. I think we are fine here, as bigint is overall an primitive not a react thing.

Is there no need to modify the documentation for the type?

good catch, @huv1k can you update docs at https://github.com/TanStack/virtual/blob/main/docs/api/virtual-item.md

Updated 🙌

@piecyk piecyk merged commit 1e12f81 into TanStack:main Aug 29, 2024
5 checks passed
@huv1k huv1k deleted the fix-react-key branch August 29, 2024 10:00
@kkx64
Copy link

kkx64 commented Sep 18, 2024

Why not just use the React type for keys?

@piecyk
Copy link
Collaborator

piecyk commented Sep 26, 2024

Why not just use the React type for keys?

As core should not be bound to any lib, adding the bigint created other issues like #819

@kkx64
Copy link

kkx64 commented Sep 26, 2024

Why not just use the React type for keys?

As core should not be bound to any lib, adding the bigint created other issues like #819

Fair enough, I forgot that the core can be used across different frameworks

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.

4 participants