Fabric (New Architecture) codegen support #3487

yungblud · 2024-01-19T07:19:28Z

Update the documentation

If you've added new functionality, update the README.md with an entry for your prop or event.
The entry should be inserted in alphabetical order.

Update the changelog

I've completed fully functioning codegen generate.

Provide an example of how to test the change

I've added yarn codgen script to package.json, so you guys can test codegen by run that script.

Focus the PR on only one area

Focused on codegen types.

Describe the changes

moved src/VideoNativeComponent.ts to src/specs/VideoNativeComponent.ts
modified react codegen types for running codegen successfully

This reverts commit fc243b0.

…gen types

src/specs/VideoNativeComponent.ts

examples/FabricExample/tsconfig.json

yungblud · 2024-01-20T04:23:20Z

@KrzysztofMoch Let me know if I have to fix or implement other things in this PR.
I've only focused for implementing codegen support in this PR.
Cheers.

…rting codegen array event params

yungblud · 2024-01-20T05:01:04Z

@KrzysztofMoch One more issue I found is "headers" on src.
In codegen types, we cannot support Record<string, string> type.
So we have to convert them as Array which has {key: string, value: string}.

I did it like this.
https://github.com/react-native-video/react-native-video/pull/3487/files#diff-70773b05560f0897b24b4baa0ed114158fd50e075c0c76de43cc30b90043eb80R16

and created one more util function to fix type issue.
https://github.com/react-native-video/react-native-video/pull/3487/files#diff-39b2554fd18da165b59a6351b1aafff3714e2a80c1435f2de9706355b4d32351R9

finally, I implemented that util function to these.
https://github.com/react-native-video/react-native-video/pull/3487/files#diff-272334bc4a560000e8736860b4e6980d3f865b9abea3baee3bfee41ab1894bd6R149
https://github.com/react-native-video/react-native-video/pull/3487/files#diff-272334bc4a560000e8736860b4e6980d3f865b9abea3baee3bfee41ab1894bd6R168

But it'll make android issue like bottom screen shot.

So I think if we want to fully support codegen, we need to change android's native codes about headers prop?
I want to know your opinion about this issue.
Cheers.

KrzysztofMoch · 2024-01-20T12:53:15Z

Yeah, seems like we can't use Record<string, string> with codegen. We should pass it as an array and then on native side convert it to Map to decrease number of changes in codebase
https://github.com/react-native-video/react-native-video/blob/3858a15b4268ae54d5b97c036d86b05aaf31bcf9/android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java#L152-L161
So, change expected type to Array and convert it to Map<String, String>, WDYT ?

yungblud · 2024-01-21T03:16:58Z

@KrzysztofMoch
Yeah, we already have code about that.
I will show you code fragment about that changes of setSrc's parsing headers part.

        Map<String, String> headers = new HashMap<>();
        ReadableArray propSrcHeadersArray = (src.hasKey(PROP_SRC_HEADERS)) ? src.getArray(PROP_SRC_HEADERS) : null;
        if (propSrcHeadersArray != null) {
            if (propSrcHeadersArray.size() > 0) {
                for (int i = 0; i < propSrcHeadersArray.size(); i++) {
                    ReadableMap current = propSrcHeadersArray.getMap(i);
                    String key = current.hasKey("key") ? current.getString("key") : null;
                    String value = current.hasKey("value") ? current.getString("value") : null;
                    if (key != null && value != null) {
                        headers.put(key, value);
                    }
                }
            }
        }

And also we should consider setDRM too. (but approach would be similar)

So if you are okay about that code fragment, I will make an additional commit about this issue.

src/types/video.ts

freeboub · 2024-01-21T20:31:44Z

src/types/video.ts

-  selectedAudioTrack?: SelectedTrack;
-  selectedTextTrack?: SelectedTrack;
-  selectedVideoTrack?: SelectedVideoTrack; // android
+  selectedAudioTrack?: SelectedTrackType;


I don't undestand why you did this change ...
SelectedTrack is an object representing all possible values to select a track where SelectedTrackType is an enum representing which kind of track selection can be used

Hmm I couldn't fully understand your point.
So you want to use typescript's enum value as selectedAudioTrack's type?

Ah, mate.
I think I just deleted typescript enum types and replaced them from codegen types to recycle codegen types.
Just naming and few types change for codegen types.
I think there would have no problem?

Let me know your opinion.
@freeboub

Here is doc:
selectedAudioTrack
Configure which audio track, if any, is played.

selectedAudioTrack={{
type: Type,
value: Value
}}

SelectedAudioTrack is an object
SelectedAudioTrack.type is the enum you use.

Is that clearer ?

We use https://github.com/react-native-video/react-native-video/pull/3487/files#diff-70773b05560f0897b24b4baa0ed114158fd50e075c0c76de43cc30b90043eb80R75 this type from 'specs/VideoNativeComponent.tsx'

We cannot use typescript's enum type with codegen. It can't be possible.
So I recycled above codegen type for Selected**Track prop types.

I think you want to consider DX for developers by handling type as enum.
I think it can be possible, but I'm not sure. I will have a try.

@freeboub Can you check this commit?
I've handled some issue about this comments.
d1a858a

I think we have to choose between

remain docs and modify some types like above commit

change docs and give up enum types

Let me know your opinion. that commit can be reverted.

src/Video.tsx

src/utils.ts

freeboub · 2024-01-21T20:43:01Z

src/specs/VideoNativeComponent.ts

@@ -0,0 +1,536 @@
+/* eslint-disable @typescript-eslint/ban-types */


Is it mandatory to move this file ?
It makes review harder ... (I think you remove and add instead of git mv)

It's because of handling {} on codegen.
I wanted to use Readonly but it cannot be possible.

But, I found another issue on OnReceiveAdEventData while I am looking this issue.
OnReceiveAdEventData['data'] type should be handled as array type I guess.
Because codegen cannot parse Record<string, string> type.
I think I just handle that type as just {} and I missed it.

Or, we have to define OnReceiveAdEventData['data'] type as Readonly object if we can define what key and value are expected.

@yungblud I think the point is that instead of deleting and adding a file (using git add), you should use git mv to move the file - this keeps the diff between files

@KrzysztofMoch Thanks. That was my mistake.
I will try that!

Hmm, I think I cannot do mv file clearly.
I tried multiple times but it's difficult to show just "mv" on file changes.

freeboub · 2024-01-21T20:44:48Z

And thank you for extracting in smaller PR ! 🙏

yungblud · 2024-01-22T03:42:53Z

And thank you for extracting in smaller PR ! 🙏

Thanks my pleasure.

KrzysztofMoch · 2024-01-22T15:08:23Z

So if you are okay about that code fragment, I will make an additional commit about this issue.

@yungblud for me its okay 🙌

…nt.ts

…-codegen-types

yungblud · 2024-01-25T05:29:46Z

@KrzysztofMoch @freeboub
67c648e
Can you guys check this new commit?
This commit is for this issue

And I would be happy if you guys check again my comments.
Thanks.

freeboub

few minor comment.
Regarding to the sample, won't it make sens to merge the new arch with basic sample?
I don't request it now, I am just asking ! I am afraid it will be painful to maintain multiple sample.

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java

src/utils.ts

yungblud · 2024-01-30T01:41:47Z

few minor comment. Regarding to the sample, won't it make sens to merge the new arch with basic sample? I don't request it now, I am just asking ! I am afraid it will be painful to maintain multiple sample.

Yeah maybe it would be happier. (if we can)
Let me consider them too. Cheers.
@freeboub

freeboub · 2024-01-30T08:24:44Z

src/types/video.ts

@@ -80,7 +80,7 @@ export enum SelectedTrackType {

 export type SelectedTrack = {
  type: SelectedTrackType;
-  value?: string | number;


Value can be a number in the code ... it is used when we select by index.
Is it problematic for codegen ? I review last commit and it seems working with Int Type !

Ah, all types in src/types/video.ts are for JS layer's prop types.
The reason I removed number is I did want to sync with codegen types of SelectedTrack.
Such as this https://github.com/react-native-video/react-native-video/pull/3487/files/d1a858a590339b48d5430fbe8b8769f8f1921bd6#diff-70773b05560f0897b24b4baa0ed114158fd50e075c0c76de43cc30b90043eb80R85

I think Codegen do not allow mixed types.

Maybe we have to change docs content of this types.
I am not sure what is the best option. But codegen do not allow mixed types.
So maybe we have to change docs?
Let me know your opinion @freeboub

ok, I can propose you this change:

keep current behavior fo external API

move the value to string only in native bindings (then string To Int in native code)

do the wrapping in Video.js to convert the int to string

This is the ideal change. but is you really prefer keep it as string only (even in external API), just notice this is a breaking change. (but you need anyway to change the native code)

okay I will have a check.
Thanks for the suggestions!

@freeboub hello, sorry for the late reply.
I've implemented your suggestions on this commit 2ef456a

Can you have a look?
If I didn't catch your suggestions, let me know.

…-codegen-types

yungblud · 2024-03-01T05:08:39Z

@freeboub Hey, thanks for the reviewing my PR.
I've resolved merge conflicts from master branch.

freeboub · 2024-03-01T07:37:21Z

This PR is OK for me @KrzysztofMoch can you please double check ?
BTW @KrzysztofMoch do you think we shall merge it before or after next release ? (I would say just after)

KrzysztofMoch · 2024-03-01T12:54:02Z

Okay, will check that
Yes, I think we should merge it and release in next release for our safety

KrzysztofMoch

I have tested both iOS & android in basic example - and it's work perfectly
Now @yungblud you can leave it to use, we will release in our time 😄
Thanks for your effort!

@yungblud I have one question, I guess that with this change we will drop support for older versions of react-native. Do you know maybe what is the older version that we can support?

yungblud · 2024-03-03T10:47:17Z

I have tested both iOS & android in basic example - and it's work perfectly Now @yungblud you can leave it to use, we will release in our time 😄 Thanks for your effort!

@yungblud I have one question, I guess that with this change we will drop support for older versions of react-native. Do you know maybe what is the older version that we can support?

@KrzysztofMoch Hey, thanks for detailed review of my PR.
Hmm, if you want to consider deprecate older versions of react-native, I think minimum "codegen available" version would be the best option.

https://reactnative.dev/blog/2022/03/30/version-068#opting-in-to-the-new-architecture
I think 0.68.0 would be the first version that RN was released with "New Architecture" support.
So I would suggest set minimum version of RN to 0.68.0 for react-native-video.

freeboub · 2024-03-06T22:08:37Z

I am ready to merge, but there are still conflicts ...
Please merge with master, it will be the next merged PR !

…-codegen-types

yungblud · 2024-03-06T22:43:16Z

@freeboub Hello! Conflicts were simple. had no problem.
I've resolved it by merging upstream/master.

KrzysztofMoch · 2024-03-07T10:36:04Z

it will be the next merged PR !

@freeboub I merged it to avoid next possible conflicts (if you are not happy with it please revert)

freeboub · 2024-03-07T19:17:27Z

@KrzysztofMoch I am happy with that :)

yungblud added 12 commits January 18, 2024 16:28

feat: implemented codegenConfig on package.json

d3d12b4

chore: moved directory location of Fabric component

f01fa46

fix: typefix FabricExample

cbbab32

chore: pod instaslled FabricExample iOS app

6f14e08

feat: implemented codegen config on package.json

25f046a

feat: implemented codegen of specs/VideoNativeComponent

84e06a3

chore: removed not using type Filter

aa03b54

feat: removed unnecessary export on codegen tyepes

fc243b0

Revert "feat: removed unnecessary export on codegen tyepes"

6deb74b

This reverts commit fc243b0.

refactor: fixed types on Video component and modified types with code…

49d3f59

…gen types

feat: modified codegenNativeComponent naming (RCTVideo)

ce872c7

feat: pod installed example basic app

6a50754

yungblud mentioned this pull request Jan 19, 2024

[New Arch] Android Fabric #3204

Open

KrzysztofMoch reviewed Jan 19, 2024

View reviewed changes

src/specs/VideoNativeComponent.ts Outdated Show resolved Hide resolved

KrzysztofMoch reviewed Jan 19, 2024

View reviewed changes

examples/FabricExample/tsconfig.json Show resolved Hide resolved

yungblud added 2 commits January 20, 2024 13:47

feat: bump up react-native dev dependency version to 0.73.2 for suppo…

0c4bd9c

…rting codegen array event params

feat: support array param types on event callback function codegen types

c9f6a92

yungblud requested a review from KrzysztofMoch January 20, 2024 04:50

chore: pod installed ios basic example

946d6a7

freeboub requested changes Jan 21, 2024

View reviewed changes

feat: modified source prop as optional type

7944195

yungblud requested a review from freeboub January 22, 2024 03:32

feat: add original src/VideoComponent.ts again

d63ac94

yungblud added 7 commits January 23, 2024 13:07

feat: add original src/VideoComponent.ts again with original file name

f503306

feat: git rm src/specs/VideoNativeComponent.ts

7ce6785

feat: git mv VideoNativeComponent.ts

10422a2

feat: git mv src/specs/VideoNativeComponent.ts

1a99d30

feat: git mv src/VideoNativeComponent.ts src/specs/VideoNativeCompone…

5287d69

…nt.ts

Merge remote-tracking branch 'upstream/master' into feature/implement…

b7db238

…-codegen-types

feat: implemented array type handling on android JAVA

67c648e

freeboub requested changes Jan 26, 2024

View reviewed changes

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java Outdated Show resolved Hide resolved

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java Outdated Show resolved Hide resolved

src/utils.ts Show resolved Hide resolved

yungblud added 3 commits January 30, 2024 13:34

feat: updated iOS requestHeaders parsing native

6c0fe68

feat: use safeGetArray on android, removed not using import too

e69666d

feat: temporary commit - reusing enum types for remaining docs types

d1a858a

freeboub reviewed Jan 30, 2024

View reviewed changes

feat: implemented mixed type of SelectedTrack.value for JS layer

2ef456a

yungblud requested a review from freeboub February 15, 2024 03:24

freeboub approved these changes Feb 25, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/implement…

941dab4

…-codegen-types

KrzysztofMoch approved these changes Mar 1, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/implement…

859cfa3

…-codegen-types

KrzysztofMoch merged commit b33e6df into TheWidlarzGroup:master Mar 7, 2024
11 checks passed

freeboub mentioned this pull request Mar 21, 2024

fix(android): track selection parameter has change in last release. #3594

Merged

		@@ -0,0 +1,536 @@
		/* eslint-disable @typescript-eslint/ban-types */

Fabric (New Architecture) codegen support #3487

Fabric (New Architecture) codegen support #3487

Conversation

yungblud commented Jan 19, 2024

Update the documentation

Update the changelog

Provide an example of how to test the change

Focus the PR on only one area

Describe the changes

yungblud commented Jan 20, 2024

yungblud commented Jan 20, 2024

KrzysztofMoch commented Jan 20, 2024

yungblud commented Jan 21, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freeboub commented Jan 21, 2024

yungblud commented Jan 22, 2024

KrzysztofMoch commented Jan 22, 2024 • edited Loading

yungblud commented Jan 25, 2024

freeboub left a comment

Choose a reason for hiding this comment

yungblud commented Jan 30, 2024

freeboub Jan 30, 2024 • 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

yungblud commented Mar 1, 2024

freeboub commented Mar 1, 2024

KrzysztofMoch commented Mar 1, 2024 • edited Loading

KrzysztofMoch left a comment • edited Loading

Choose a reason for hiding this comment

yungblud commented Mar 3, 2024

freeboub commented Mar 6, 2024 • edited Loading

yungblud commented Mar 6, 2024

KrzysztofMoch commented Mar 7, 2024 • edited Loading

freeboub commented Mar 7, 2024

yungblud commented Jan 21, 2024 •

edited

Loading

KrzysztofMoch commented Jan 22, 2024 •

edited

Loading

freeboub Jan 30, 2024 •

edited

Loading

KrzysztofMoch commented Mar 1, 2024 •

edited

Loading

KrzysztofMoch left a comment •

edited

Loading

freeboub commented Mar 6, 2024 •

edited

Loading

KrzysztofMoch commented Mar 7, 2024 •

edited

Loading