Skip to content

TypeScript typings are swallowed for most of the components #1758

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

Closed
yamafaktory opened this issue Apr 1, 2021 · 6 comments
Closed

TypeScript typings are swallowed for most of the components #1758

yamafaktory opened this issue Apr 1, 2021 · 6 comments

Comments

@yamafaktory
Copy link

🐛 Bug Report

Hi folks 👋,

First of all, thanks for the massive work done on this project!

Back to the bug report: there's an issue with TypeScript's types for most of the components. It seems like React.forwardRef is the culprit here.

🤔 Expected Behavior

All the components should expose their types correctly - i.e. no any.

😯 Current Behavior

For example, the following View component - when imported - is inferred as any:

image

https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/view/src/View.tsx#L45

Whereas a Button works totally fine:

image

https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/button/src/Button.tsx#L96

💁 Possible Solution

Enforce proper types exposition (via aliasing).

🔦 Context

I encountered that while using React Spectrum in production project.

💻 Code Sample

Not relevant.

🌍 Your Environment

Software Version(s)
react-spectrum 3.9.0
typescript 4.2.3
Browser not relevant
Operating System not relevant

🧢 Your Company/Team

Not relevant.

🕷 Tracking Issue (optional)

None.

@LFDanLu
Copy link
Member

LFDanLu commented Apr 14, 2021

I believe you are correct with forwardRef being the culprit, think we need to add something like as (props: ViewProps & {ref: DOMRef}) => ReactElement; to View.

@LFDanLu LFDanLu added the good first issue Good for newcomers label May 19, 2021
@LFDanLu
Copy link
Member

LFDanLu commented May 24, 2021

@yamafaktory Apologies, but "present" me doesn't remember what reproduction steps "past" me did to figure out the above fix. I made a codesandbox here: https://codesandbox.io/s/sparkling-butterfly-1gvyr?file=/src/App.tsx and it looks like View has the proper typing here. Does your project have any additional configurations that are different from this sandbox?

majornista added a commit to majornista/react-spectrum-v3 that referenced this issue May 25, 2021
@yamafaktory
Copy link
Author

@LFDanLu Thanks a ton for the feedback and effort!

Indeed your sandbox works fine. I tried to reproduce it - i.e. make it work in my private project - by upgrading to the latest TS version and React types without success. Then I noticed that the real issue is that React Spectrum uses https://www.typescriptlang.org/tsconfig#allowSyntheticDefaultImports set to true (https://github.com/adobe/react-spectrum/blob/main/tsconfig.json#L11) and then you're doing all the React imports like e.g. https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/view/src/View.tsx#L16.

So basically, with allowSyntheticDefaultImports set to false, the React import doesn't resolve correctly for the types:

image

It should be done like that:

image

This is also the recommended way to do it since React as no more default export, see facebook/react#18102.

TL;DR: either folks like me have to set allowSyntheticDefaultImports to true, or React Spectrum should update all the imports accordingly.

N.B.: I'm not using Babel to transpile the code but ESBuild, that's why I was not using this option, see again https://www.typescriptlang.org/tsconfig#allowSyntheticDefaultImports:

For convenience, transpilers like Babel will automatically create a default if one isn’t created

@LFDanLu
Copy link
Member

LFDanLu commented May 28, 2021

Thanks for all the investigation! Will make a note to bring this up in our next planning.

@yamafaktory
Copy link
Author

Thanks a ton @LFDanLu 🙇!

@dannify dannify removed the good first issue Good for newcomers label Dec 7, 2021
@devongovett
Copy link
Member

Pretty sure this is no longer an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants