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

feat: ts #20

Merged
merged 14 commits into from
Oct 22, 2024
Merged

feat: ts #20

merged 14 commits into from
Oct 22, 2024

Conversation

headfire94
Copy link
Contributor

@headfire94 headfire94 commented Oct 17, 2024

depends on #19

convert to TS.

breaking: removed generics types usage.
reason:
We shouldn't use shakl with custom props like:

const StyledText = styled.Text((props) => props.custom ? ({flex: 1, width: 10}) : ({width: 10}) )

return <StyledText custom={true} />

Instead we should create component that uses dynamic style.

const StyledText = styled.Text({width: 10})

return <Text style={custom ? {flex: 1} : {}} />

The reason is that with shakl with pass this custom prop down to component - this is not expected. right now react-native allows that, but it already breaks typings, native components don't allow to pass random props

We still can use it with native props:

const StyledTouchable = styled.Touchable((props) => props.disabled ? ({flex: 1, width: 10}) : ({width: 10}) )

return <StyledTouchable disabled={true} />

@sonaye
Copy link
Collaborator

sonaye commented Oct 18, 2024

This is a major breaking change, dynamic props are an intentional API design choice, this will likely break hundreds of components, what benefits do we get from it?

const StyledText = styled.Text((props) => props.custom ? ({flex: 1, width: 10}) : ({width: 10}) )

^^ this isn't the typical usage though, usually it's something like this

const Label = styled.Text((props) => ({ color: props.positive ? 'green' : 'black' })) // <Label positive />

@headfire94
Copy link
Contributor Author

headfire94 commented Oct 18, 2024

Not a breaking change (for JS), you still can do it but TS don't support it (and never did that) because of the reasons I described above

@headfire94
Copy link
Contributor Author

To properly support it in TS we need logic that will omit unnecessary props you pass down to native component which are not supported

@headfire94
Copy link
Contributor Author

headfire94 commented Oct 18, 2024

As i understand there is only one option to by pass unexpected props - it requires overriding native type to any (but we loose all benefit from TS then):

import { Text } from 'react-native';

// bypass TypeScript's
const CustomText = Text as any;

const App = () => (
  <CustomText customProp="test-value">
    Hello, World!
  </CustomText>
);

cc @sparten11740 if you know some other ts tricks

src/index.tsx Outdated
Comment on lines 82 to 123
if (styleFromProps) {
if (
typeof styleFromProps === 'number' ||
(styleFromProps as any).hasOwnProperty('viewDescriptors')
) {
style = [style, styleFromProps, fixedStyle];
} else if (Array.isArray(styleFromProps)) {
style = [style, ...styleFromProps, fixedStyle];
} else {
style = {
...style,
...styleFromProps,
...fixedStyle,
};
}
}

const parentProps = {
...factoryProps,
...attrsResult,
...restProps,
style,
};

return React.createElement(
comp || Comp,
{ ref, ...parentProps },
child
? React.createElement(
child,
{
ref: childRef,
...(typeof childProps === 'function'
? childProps(parentProps)
: childProps),
},
children
)
: children
);
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we additionally memoize these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think better in separate PR. benchmarks may change

Copy link
Contributor

@sparten11740 sparten11740 left a comment

Choose a reason for hiding this comment

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

I proposed a way to keep the style props. I think usage such as <SyledText customProp={} ... /> is idiomatic:

#21

src/index.tsx Outdated
Comment on lines 106 to 123
return React.createElement(
comp || Comp,
{ ref, ...parentProps },
child
? React.createElement(
child,
{
ref: childRef,
...(typeof childProps === 'function'
? childProps(parentProps)
: childProps),
},
children
)
: children
);
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this to be a .tsx file, we're explicitly not using jsx, right?

@headfire94 headfire94 changed the base branch from egor/fix/rn-stylesheet to ain/perf October 18, 2024 20:52
@headfire94
Copy link
Contributor Author

headfire94 commented Oct 18, 2024

@sparten11740 if default prop defined in attrs, but required by native component. How to tell TS that we already provided it in attrs ?

Example:

Index: package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/package.json b/package.json
--- a/package.json	(revision 3bd38d70b7ee2d2c8cf856a1c32f0dadc9a4f4a4)
+++ b/package.json	(date 1729284853712)
@@ -28,6 +28,7 @@
     "microtime": "^3.1.1",
     "react": "18.2.0",
     "react-native": "^0.73.6",
+    "react-native-linear-gradient": "^2.8.3",
     "react-native-reanimated": "^3.8.1",
     "react-test-renderer": "18.2.0",
     "rollup": "^4.13.2",
Index: test/index.type-test.tsx
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/index.type-test.tsx b/test/index.type-test.tsx
--- a/test/index.type-test.tsx	(revision 3bd38d70b7ee2d2c8cf856a1c32f0dadc9a4f4a4)
+++ b/test/index.type-test.tsx	(date 1729284962933)
@@ -1,7 +1,7 @@
-import React, { Text } from 'react-native'
+import React, {Text, ViewStyle} from 'react-native'
 import styled from '../src/index'
 import extendedStyled from '../src/rn'
-
+import LinearGradient, { LinearGradientProps } from 'react-native-linear-gradient'
 const StyledText = styled(Text)(({ transparent }: { transparent?: boolean; color: string }) => ({ flex: 1, opacity: transparent ? 0.5 : 1 }))
 const StyledScalableText = StyledText.extend(({ big }: { big?: boolean }) => ({ fontSize: big ? 20 : 10 }))
 const StyledTextWithObjectProps = styled(Text)({ flex: 1, opacity: 1 })
@@ -30,6 +30,16 @@
 const StyledTouchableWithDynamicProps = extendedStyled.Image((props: {active: boolean}) => ({width: props.active ? 100 : 50}))
 const ViewWithText = extendedStyled.View({}).withChild(StyledText)
 
+const extendedWithLinear = Object.assign(extendedStyled, {
+    LinearGradient: styled<LinearGradientProps, ViewStyle>(LinearGradient, {
+        name: 'styled(LinearGradient)',
+    })
+})
+
+const StyledLinear = extendedWithLinear.LinearGradient({}).attrs({
+    colors: ['red', 'white']
+})
+
 const MyScreen = () => {
   return (
     <>
@@ -63,6 +73,8 @@
       <StyledTouchableWithDynamicProps active>Touchable with dynamic style</StyledTouchableWithDynamicProps>
 
       <ViewWithText />
+
+        <StyledLinear />
     </>
   )
 }

TS2741: Property colors is missing in type {} but required in type LinearGradientProps

I managed to fix it with

 styled(
  LinearGradient as React.ComponentType<
    Omit<LinearGradientProps, 'colors'> & {
      colors?: LinearGradientProps['colors']
    }
  >
)({}).attrs({colors: ['red', 'white']})

is there a better alternative to this approach?
UPD: can be added in next PR, this one is already better than we had before

@headfire94 headfire94 marked this pull request as ready for review October 21, 2024 15:34
@headfire94 headfire94 marked this pull request as draft October 21, 2024 15:35
@headfire94 headfire94 marked this pull request as ready for review October 21, 2024 15:38
Copy link
Contributor

@sparten11740 sparten11740 left a comment

Choose a reason for hiding this comment

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

utACK

@sparten11740 sparten11740 mentioned this pull request Oct 21, 2024
@headfire94
Copy link
Contributor Author

@alexandrius please release once approve, i don't have access

Copy link
Contributor

@alexandrius alexandrius left a comment

Choose a reason for hiding this comment

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

utACK

@alexandrius alexandrius merged commit 2df1b02 into ain/perf Oct 22, 2024
@alexandrius
Copy link
Contributor

@headfire94 published

@sparten11740 sparten11740 deleted the egor/feat/ts branch October 22, 2024 09:46
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