Typescript #734

Titozzz · 2020-08-01T15:01:06Z

2 'bugs' might have been fixed btw

Platforms affected

What does this PR do?

What testing has been done on this change?

Tested features checklist

2 'bugs' might have been fixed btw

bd-arc · 2020-08-02T05:11:08Z

@Titozzz This is looking great, kudos to you 👍

The only issue is that some of the code styling rules you've used are absolute no-go's for us — the rules we use are opinionated and enforced company-wide.

Particularly:

4 spaces for indentation.
Space required before methods' parenthesis.
Ternaries are written this way:

let nextActiveItem =
    typeof this._activeItem !== 'undefined' ?
        this._activeItem :
        nextFirstItem;

Apart from that, excellent job so far!

Can you tell me why you've removed the eslint file?

Titozzz · 2020-08-02T08:45:19Z

I just aligned the repo to the default config from eslint community. No problem, I'll update it back

Titozzz · 2020-08-05T23:20:29Z

I tried reverting to your config + adding some typescript defaults. Lemme know if that need any more changes

Titozzz · 2020-08-05T23:49:14Z

I've made sure the example works, let me know if I need to test more

bd-arc · 2020-08-06T04:34:44Z

@Titozzz Amazing work, thank you so much 🙏

I'll set aside some time this weekend to review it in length. Anything particular I should know about using https://github.com/react-native-community/bob?

Titozzz · 2020-08-08T09:53:44Z

Nope it's just used in the prepare phase to build the code

narender2031 · 2020-08-13T04:26:30Z

src/parallaximage/ParallaxImage.tsx

+  offset: number;
+  width: number;
+  height: number;
+  status: 1 | 2 | 3 | 4;


I think we can also define the types of these numbers. It will be easy to understand when someone is trying to use it.

I tried being as precise as I could and 1|2|3|4 is usable by a function expecting number but it's also more clear

I'm ok with keeping the numbers as is. However we should probably copy the following comment there as well for clarity's sake:

// 1 -> loading | 2 -> loaded | 3 -> transition finished | 4 -> error

@bd-arc & @Titozzz we can do one more thing. We can add the meaning of the numbers in the documentation. So that users can understand the meaning of these numbers.

I can probably replace with an enum. That'll make things easier

@Titozzz Great idea!

narender2031 · 2020-08-13T04:32:08Z

src/pagination/Pagination.tsx

+  activeDotIndex: number;
+  dotsLength: number;
+  activeOpacity?: number;
+  carouselRef?: Carousel<unknown> | null;


carouselRef?: Carousel<any> | null; I think we can replace unknown with any. Please share your thoughts regarding this.

I tried avoiding all "any's". I suppose I could also template this so that it would be TData if you prefer

@Titozzz Let's go with TData if you don't mind :-)

narender2031 · 2020-08-13T04:36:29Z

@Titozzz @bd-arc TypeScript looks fine to me. add a few suggestions. Please take look and share your thoughts.

narender2031 · 2020-08-13T04:39:33Z

src/parallaximage/ParallaxImage.tsx

+import type Carousel from 'src/carousel/Carousel';
+
+type ParallaxImageProps = {
+  carouselRef: Carousel<unknown> | null; // passed from <Carousel />


As I mention above, If we can replace Unknown with any. Then please change here also.

Same answer as Pagination

Again, let's use TData here as well.

bd-arc · 2020-08-13T16:16:45Z

@Titozzz Just went over the whole PR and, as far as I can tell, you've done an astounding job! Thank you so much 🙏

Let me know once you're done with the really minor corrections and then... Let's get this merged!

PS — Thanks a lot @narender2031 for your thorough review 🙂

nelsonprsousa · 2020-08-15T01:54:17Z

Hey guys,

I would like to test this. Currently I have "react-native-snap-carousel": "4.0.0-beta.5" on my package.json. What should I do to test this PR?

Maybe with the last commit hash? 🤔

Thank you

bd-arc · 2020-08-15T05:46:34Z

@nelsonprsousa You can target any commit of the repo with its SHA.

Here's an example with this PR's current latest commit:

"react-native-snap-carousel": "https://github.com/archriss/react-native-snap-carousel#e719467"

nelsonprsousa · 2020-08-15T11:58:42Z

I tried it, but got this error:

I am sure I am missing something here 🤔

Titozzz · 2020-08-15T17:03:31Z

I tried it, but got this error:

I am sure I am missing something here 🤔

Please try cloning it from my fork

nelsonprsousa · 2020-08-15T17:32:32Z

Please try cloning it from my fork

Seems to work with your fork. However, once I yarn I get this error:

Here's my package.json: https://gist.github.com/nelsonprsousa/61ef81bdddc0823bb9ec793924d4c22b

Do you know if this is a local related problem?

Titozzz · 2020-08-18T12:29:43Z

@nelsonprsousa Fixed it, thanks :)

I uploaded a packed version if you want to try it out just download and you can add it with yarn add "path"
https://github.com/Titozzz/files/blob/master/react-native-snap-carousel-v4.0.0-beta.5.tgz

@bd-arc @narender2031 fixes are done, can't do many tests as I don't have a good connection here, but should be just fine.
Please test 🚀

nelsonprsousa · 2020-08-18T13:21:24Z

@nelsonprsousa Fixed it, thanks :)

I uploaded a packed version if you want to try it out just download and you can add it with yarn add "path"
https://github.com/Titozzz/files/blob/master/react-native-snap-carousel-v4.0.0-beta.5.tgz

@bd-arc @narender2031 fixes are done, can't do many tests as I don't have a good connection here, but should be just fine.
Please test 🚀

Now I could add it 👍
Already tested our application with this version and it is working as expected. 🚀 We are using the Carousel and Pagination, too.

I was also expecting to have types with the migration to TypeScript. Am I missing something here? Maybe we need to do something else?

Nevertheless the runtime app looks 👌

Titozzz · 2020-08-18T19:02:52Z

Thanks for testing, I fcked up the path to the d.ts file 🙄 , now it is fixed @nelsonprsousa

Feel free to edit the package.json if you don't wanna reinstall 😛

nelsonprsousa · 2020-08-18T19:53:57Z

Thanks for testing, I fcked up the path to the d.ts file 🙄 , now it is fixed @nelsonprsousa

Feel free to edit the package.json if you don't wanna reinstall 😛

Types are now working 👌 Awesome job! 🚀

Just find it hard to compile it without any error 😅

This is my code:

const renderItem = (baseData: {
    index: number;
    dataIndex: number;
    item: IPointOfSale;
}): ReactNode => (
    <Card
        title={baseData.item.title}
        subtitle={baseData.item.subtitle}
        imagePath={baseData.item.imagePath}
        width={96}
        height={25}
        onPress={navigate(navigation, baseData.item.navigation)}
    />
);

<Carousel
    ref={carouselRef}
    data={pointsOfSale}
    renderItem={renderItem}
    onScrollIndexChanged={setActiveSlide}
    sliderWidth={theme.mixins.width}
    itemWidth={theme.mixins.width}
    hasParallaxImages
    containerCustomStyle={{
        overflow: 'visible',
        marginLeft: theme.mixins.widthPercentageToDP(2),
    }}
    inactiveSlideScale={0.65}
    inactiveSlideOpacity={0.35}
/>

But I am getting this:
Type '(baseData: { index: number; dataIndex: number; item: IPointOfSale;}) => ReactNode' is not assignable to type '(baseData: { index: number; dataIndex: number; item: unknown; }, parallaxData?: { scrollPosition: Value | undefined; carouselRef: ScrollView | FlatList<unknown> | null; vertical?: boolean | undefined; sliderWidth?: number | undefined; sliderHeight?: number | undefined; itemWidth?: number | undefined; itemHeight?: nu...'. Types of parameters 'baseData' and 'baseData' are incompatible. Type '{ index: number; dataIndex: number; item: unknown; }' is not assignable to type '{ index: number; dataIndex: number; item: IPointOfSale; }'. Types of property 'item' are incompatible. Type 'unknown' is not assignable to type 'IPointOfSale'.ts(2322)

Maybe the problem is the item's type? 🤔

Titozzz · 2020-08-20T21:49:48Z

I'll do some tests and come back at you

nelsonprsousa · 2020-08-21T16:17:23Z

Hey guys, quick update on my side after testing:
All working now 🚀

Thanks @Titozzz for your time and patience with this awesome PR! You rock 🚀

@bd-arc can we merge this to v4 branch to test as v4.0.0-beta.6?

Titozzz · 2020-08-21T17:46:02Z

I need to push a few fixes to parallax image,but then yeah we should merge

bd-arc · 2020-08-22T07:02:21Z

@Titozzz Can't thank you enough for your amazing work on this one 👍

@nelsonprsousa That's exactly the plan! As soon as @Titozzz tells me it's ready, I'll publish a new release :-)

Titozzz · 2020-08-24T14:19:26Z

I've had to make a few key changes to make sure types would be as close to reality as they needed to be.
Most library don't bother doing this but it will make the user experience better.
We could have typed the carousel like that:

{
  vertical?: boolean,
  height?: number,
  width?: number,
}

That's the easy way.
Instead I've gone

{
  vertical: false,
  width: number
} | {
  vertical: true,
  height: number,
}

Which is more accurate, and doesn't allow for user error.
The drawback is now that when using TypeScript you need to specify vertical={false} (or true) explicitly.
I've tried for a long time to make it default to horizontal so that user would have the horizontal types when not specifying but that is just not possible with the current TS version.
Note that this doesn't impact the JS users, and is not a breaking change for them. Only TS users will see a difference.
I've also removed the hasParallaxImages prop, and I'm now always passing a second argument, this has no perf impact, and vastly simplified the types. Now the function renderItem is properly typed depending on vertical.

There are plenty of things I'd like to go over but this PR has waited enough and code is working fine as is.

Unless anyone comments otherwise, has bugs, this is READY TO MERGE @bd-arc

bd-arc · 2020-08-26T16:56:23Z

This is superb! I'll do a few more tests and will most certainly merge and publish your PR tomorrow.

Thank you for your awesome job on this one @Titozzz 👍

Titozzz · 2020-08-26T17:06:36Z

Can't wait to get some feedbacks on this. I'm playing with reanimated 2 right now

bd-arc · 2020-08-27T16:03:11Z

@Titozzz Merged and published!

Two observations:

The code styling you've used is pretty far from what we enforce in the company and what is defined in the .eslintrc file (I've just updated it by the way). For some reason, I can no longer get ESLint to work and autofix as much of it as possible. Better luck on your end?
Nice to hear about Reanimated 2. The plugin could really use an addition like this one further down the road ;-)

Titozzz · 2020-08-27T17:08:43Z

@bd-arc Any way we can chat, other that using github issues ? :)

Titozzz · 2020-08-27T17:39:48Z

#750 for 1)

Aryk · 2020-09-29T10:50:21Z

I noticed that the Carousel component is missing some props from "InheritedPropsFromFlatlist", namely extraData which I required on my component when the size changes (lets say a photo finishes uploading).

Should I make a separate ticket for that?

Aryk · 2021-04-05T06:58:25Z

@Titozzz

I was looking at this again. Few things:

Why are we using a "type" for CarouselProps instead of "interface"? I can't extend from CarouselProps to create other interfaces because it is not a static type. I believe in the @types version it was an interface.
Why are optional props listed as "required". My guess is that that type is used internally in the library but most of those values are defaulted by the library. To the user, they should all be optional.
renderItem requires "parallaxData" as the second argument but I don't believe it's required by the library to work.
Switch vertical=true and vertical=false doesn't get me valid type checking for the other attributes like sliderWidth. I'll see vertical={false} and it will still blow up saying it couldn't find sliderWidth in the types. I believe you used interfaces I believe this would work properly.

Is there another way I should be looking at getting typescript support for 4.0.0-beta?

bd-arc · 2021-04-05T09:52:16Z

@Aryk Thanks for the feedback! @Titozzz did an amazing job migrating everything to TypeScript, but we know that a few things probably need some fine-tuning.

You're more than welcome to update the Typescript definition and submit a PR if you feel like it. Just make sure to ping me when you do so or I'm probably going to miss it.

Titozzz · 2021-04-05T09:59:20Z

There is definitely a big need for polishing. I'll try fixing this in the next weeks

Typescript

wip

2300990

2 'bugs' might have been fixed btw

Titozzz changed the title ~~🚧 wip(Carousel): huge revamp of carousel's logic, props and behavior~~ WIP: Typescript Aug 1, 2020

Titozzz mentioned this pull request Aug 1, 2020

[Help wanted] Looking for maintainers #632

Open

Titozzz added 2 commits August 6, 2020 01:16

wip

9f0dfe6

Delete whatthehell.js

bc86b88

Titozzz added 2 commits August 6, 2020 01:30

Update animations.ts

b96c5f4

Update Example folder

e719467

Titozzz marked this pull request as ready for review August 5, 2020 23:49

Titozzz changed the title ~~WIP: Typescript~~ Typescript Aug 5, 2020

bd-arc mentioned this pull request Aug 10, 2020

🔥 4.0.0 beta.x #678

Open

narender2031 approved these changes Aug 13, 2020

View reviewed changes

narender2031 suggested changes Aug 13, 2020

View reviewed changes

Titozzz added 2 commits August 18, 2020 13:34

wip

5f8ad4a

diverse fixes

daa8e6e

Update package.json

f403a9a

Titozzz added 2 commits August 21, 2020 16:48

wip

93fb6a0

Update Pagination.tsx

0aee79e

Finally it should be good

12a2ea5

bd-arc merged commit 089ecd3 into meliorence:4.0.0-beta.x Aug 27, 2020

itenl pushed a commit to itenl/react-native-snap-carousel that referenced this pull request Aug 27, 2021

Merge pull request meliorence#734 from Titozzz/typescript

0c3ac68

Typescript

Typescript #734

Typescript #734

Conversation

Titozzz commented Aug 1, 2020

Platforms affected

What does this PR do?

What testing has been done on this change?

Tested features checklist

bd-arc commented Aug 2, 2020

Titozzz commented Aug 2, 2020

Titozzz commented Aug 5, 2020

Titozzz commented Aug 5, 2020

bd-arc commented Aug 6, 2020

Titozzz commented Aug 8, 2020 • edited Loading

narender2031 Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narender2031 commented Aug 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bd-arc commented Aug 13, 2020

nelsonprsousa commented Aug 15, 2020 • edited Loading

bd-arc commented Aug 15, 2020

nelsonprsousa commented Aug 15, 2020

Titozzz commented Aug 15, 2020

nelsonprsousa commented Aug 15, 2020

Titozzz commented Aug 18, 2020

nelsonprsousa commented Aug 18, 2020

Titozzz commented Aug 18, 2020 • edited Loading

nelsonprsousa commented Aug 18, 2020 • edited Loading

Titozzz commented Aug 20, 2020

nelsonprsousa commented Aug 21, 2020 • edited Loading

Titozzz commented Aug 21, 2020

bd-arc commented Aug 22, 2020

Titozzz commented Aug 24, 2020

bd-arc commented Aug 26, 2020

Titozzz commented Aug 26, 2020

bd-arc commented Aug 27, 2020

Titozzz commented Aug 27, 2020

Titozzz commented Aug 27, 2020

Aryk commented Sep 29, 2020

Aryk commented Apr 5, 2021 • edited Loading

bd-arc commented Apr 5, 2021

Titozzz commented Apr 5, 2021

Titozzz commented Aug 8, 2020 •

edited

Loading

narender2031 Aug 13, 2020 •

edited

Loading

nelsonprsousa commented Aug 15, 2020 •

edited

Loading

Titozzz commented Aug 18, 2020 •

edited

Loading

nelsonprsousa commented Aug 18, 2020 •

edited

Loading

nelsonprsousa commented Aug 21, 2020 •

edited

Loading

Aryk commented Apr 5, 2021 •

edited

Loading