adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.96k stars 1.13k forks source link

Type of Key in react-aria does not match type of Key in React #5939

Open vbudovski opened 8 months ago

vbudovski commented 8 months ago

Provide a general summary of the issue here

The type of Key in react-aria is defined as:

export type Key = string | number;

while React defines it as:

type Key = string | number | bigint;

Is there a reason to not use the React definition of the key? You can re-export it from react-aria. If you do wish to re-define it, perhaps it would be good to add some typechecks to react-aria to make sure they are interchangable at least.

e.g.

type AriaKey = string | number;
type Intersection = ReactKey & AriaKey;
let reactKey: ReactKey;
// This line will produce a TypeScript error if they don't overlap.
const intersection: Intersection =
    // @ts-ignore We don't care about this error.
    reactKey;

πŸ€” Expected Behavior?

React Key can be passed to props expecting a unique ID/key.

😯 Current Behavior

There's a type mismatch and TypeScript complains when you pass a React Key to id prop for instance.

πŸ’ Possible Solution

See above.

πŸ”¦ Context

No response

πŸ–₯️ Steps to Reproduce

import type { Key } from 'react';

const id: Key = '123'

<Tag
    // @ts-ignore FIXME: Incorrect type in react-aria-components.
    id={id}
>
  Hello
</Tag>

Version

RAC 1.1.1

What browsers are you seeing the problem on?

Other

If other, please specify.

TypeScript issue.

What operating system are you using?

Mac OS

🧒 Your Company/Team

No response

πŸ•· Tracking Issue

No response

snowystinger commented 8 months ago

Great question. We moved away from their definition because they introduced a breaking change to it in a minor. However, we were already thinking of moving away from it as it isn't really the same thing as in React and it didn't make sense to be tied to their definition.

vbudovski commented 8 months ago

Thanks for that. Would it be better to rename it then? Any reason not to include bigint as well? Some additional documentation highlighting this type would be helpful.

snowystinger commented 8 months ago

We can't rename it without a big breaking change. Though we're slowly moving away from it with continued support. We didn't include BigInt because we have not tested with those values.

Kadrian commented 6 months ago

Is there anything you'd recommend we do in the mean time? Should we simply add @ts-expect-error to all places we encouter this error? Or is there a way to globally ignore this somehow?

snowystinger commented 6 months ago

It'd be best for future updates if you changed to import {Key} from 'react-aria-components'; You can rename it if you like as well, import {Key as RACKey} from 'react-aria-components';

There may be some libraries that allow you to ignore certain ts codes as well.