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

Intellisense doesn't work for some props because TypeScript for these props are not correct. #5622

Open
5 tasks
bao-multiIT opened this issue Dec 25, 2022 · 4 comments

Comments

@bao-multiIT
Copy link

Description

Intellisense doesn't work for some props because TypeScript for these props are not correct.

CodeSandbox/Snack link

No need.

Steps to reproduce

Props like justifyContent, alignItems, alignSelf, alignContent, flexDirection, flexWrap have their type as ResponsiveValue<(string & {}) | (number & {})>, which doesn't work with Intellisense for suggesting because basically they're considered dynamic strings.

Steps to reproduce:

  1. Use any component that has the props above
  2. Type that prop and wait for suggestion
  3. See that there are no suggestions, even though there should be like the original React Native style props.

They should suggest as follow:

  • justifyContent: "flex-start" | "flex-end" | "center" | "space-between" | "space-around" | "space-evenly"
  • alignItems: "stretch" | "flex-start" | "flex-end" | "center" | "baseline"
  • alignSelf: same as alignItems
  • alignContent: "flex-start" | "flex-end" | "stretch" | "center" | "space-between" | "space-around"
  • flexDirection: "column" | "row" | "column-reverse" | "row-reverse"
  • flexWrap: "wrap" | "nowrap"

TypeScript:
image

No suggestions:
image

NativeBase Version

3.4.25

Platform

  • Android
  • CRA
  • Expo
  • iOS
  • Next

Other Platform

Visual Studio Code

Additional Information

No response

@AshwiniKumar007
Copy link

Hey @bao-multiIT,
Thanks for reporting. We will look into this.

@AlanSl
Copy link

AlanSl commented Jan 25, 2023

@AshwiniKumar007 I think the cause of this is here: https://github.com/GeekyAnts/NativeBase/blob/master/src/theme/types.ts#L14-L20

type GetRNStyles<key, scale = null> = scale extends keyof ITheme
  ? GetThemeScaleValues<scale>
  : key extends keyof CSSProperties
  ? ResponsiveValue<CSSProperties[key]>
  : key extends keyof RNStyles
  ? ResponsiveValue<RNStyles[key]>
  : unknown;

These are style keys that exist in both React Native's RNStyles and csstype's CSSProperties. GetRNStyles above chooses the (very permissive) csstype version of the type, ignoring the React Native style type, meaning values that are invalid in React Native (and may cause a hard app crash in Android) are misleadingly treated as if they're valid.

I think you can fix this by switching the order of those conditions, so if a type exists in React Native, that's the one that is chosen:

type GetRNStyles<key, scale = null> = scale extends keyof ITheme
  ? GetThemeScaleValues<scale>
  : key extends keyof RNStyles // Choose react native's type if available
  ? ResponsiveValue<RNStyles[key]>
  : key extends keyof CSSProperties // Fall back to CSS type if this is web-only
  ? ResponsiveValue<CSSProperties[key]>
  : unknown;

@seyaobey-dev
Copy link

Hi, facing same issue.

Because intellisense is not working properly, we accidentally misspelled a (string) and pushed the change. The app started crashing in production. We spend a few days to locate this error.

Please, thank you for looking into this issue so that typescript can properly help us prevent these errors.

Thank you the Native Base team for the excellent job!.

@AlanSl
Copy link

AlanSl commented Mar 9, 2023

@AshwiniKumar007 Thinking about this again, why is CSSProperties used for generic top-level NativeBase styles at all? Every key and value it adds that wasn't already allowed by RNStyles will cause Android to crash.

Yes, those extra styles can be useful within React Native Web context - but isn't that what the _web prop is for? Shouldn't CSSProperties only be applied to the allowed types within _web, where it is safe and will work?

Surely top-level style props should only allow safe React Native style values that won't cause crashes on Android, and so the typing should nudge people to, for example, replace this:

// Works on Web, fails on iOS, crashes the app on Android.
// Is allowed by current NativeBase types via merging in `CSSProperties` - but it probably shouldn't be.
<Box justifyContent="safe center">

...with this (after moving CSSProperties types to extend RNStyles only within _web props):

// Works everywhere
<Box justifyContent="center" _web={{ justifyContent: 'safe center' }}>

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