[Codegen 102]: merge getExtendsProps & getProps fns - Flow

[Pranav-yadav]

Summary:

[Codegen 102] This PR is subtask of umbrella #34872. It extracts the code to compute the extendsProps and the props properties in Flow in a getProps() -> {extendsProps, props} function into the same index.js file. This will help unifying the buildComponentSchema functions between Flow and TS so we can factor it out in a later step.

Changelog:

[INTERNAL][CHANGED] - merge getExtendsProps & getProps fns into getProps fn - Flow.

Test Plan:

• yarn flow && yarn test react-native-codegen --> should be green.

[Pranav-yadav]

... getProps() -> {extendsProps, props} function into the sameindex.js file.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

[Pranav-yadav]

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

[cipolleschi]

Hi @Pranav-yadav, thanks for taking the time to look into this. You can also look at the twin PR here for more inspiration.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

We can move both to props.js so that the index.js is thinner

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

Yeah, tests are failing. Do they fail locally for you or do they pass locally?
In theory, this is just a move, so they have to keep pass everywhere.
The error says:

GenerateViewConfigJs can generate for 'EnumPropNativeComponent.js'
Failed to find type definition for "ViewProps", please check that you have a valid codegen flow file
24 | return typeAlias.right.typeParameters.params[0].properties;
25 | } catch (e) {
> 26 | throw new Error(
| ^
27 | `Failed to find type definition for "${typeName}", please check that you have a valid codegen flow file`,
28 | );
29 | }

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

[cipolleschi]

Hi @Pranav-yadav! I pointed out some changes to help you mov forward.

Let me know if you need further help here!

[Pranav-yadav]

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

Thanks @cipolleschi. I started again with atomic changes and to make sure everything works as before, eventually I had to move the entire extends.js into props.js.

PS: If we still want to keep extends.js it'll add extra overhead, also eventually we'll need all of it's logic in props.js :)

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,622,804	+0
android	hermes	armeabi-v7a	7,936,158	+0
android	hermes	x86	9,109,453	+0
android	hermes	x86_64	8,964,474	+0
android	jsc	arm64-v8a	9,187,054	+0
android	jsc	armeabi-v7a	8,377,930	+0
android	jsc	x86	9,245,111	+0
android	jsc	x86_64	9,503,774	+0

Base commit: cff4bc8
Branch: main

[Pranav-yadav]

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

The rebase comments didn't work. Strange! 🤔

Btw, looks like you missed the following comment;

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[Pranav-yadav]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

ping @cipolleschi whenever you get time have a look at it.

[Pranav-yadav]

/rebase

Reason: CI seems to be fixed!

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

Hi @Pranav-yadav, you can see the engine we support from the package.json. Apps created from the template have a similar block in their package.json

[Pranav-yadav]

Thanks.
I had checked earlier that the root package.json defines >=16.
Just wasn't sure about the rest of the packages under RN.

[facebook-github-bot]

@cipolleschi merged this pull request in efc6e14.

[Pranav-yadav]

Summary:

[Codegen 102] This PR is subtask of umbrella #34872. It extracts the code to compute the extendsProps and the props properties in Flow in a getProps() -> {extendsProps, props} function into the same index.js file. This will help unifying the buildComponentSchema functions between Flow and TS so we can factor it out in a later step.

Changelog:

[INTERNAL][CHANGED] - merge getExtendsProps & getProps fns into getProps fn - Flow.

Test Plan:

• yarn flow && yarn test react-native-codegen --> should be green.

[Pranav-yadav]

... getProps() -> {extendsProps, props} function into the sameindex.js file.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

[Pranav-yadav]

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

[cipolleschi]

Hi @Pranav-yadav, thanks for taking the time to look into this. You can also look at the twin PR here for more inspiration.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

We can move both to props.js so that the index.js is thinner

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

Yeah, tests are failing. Do they fail locally for you or do they pass locally?
In theory, this is just a move, so they have to keep pass everywhere.
The error says:

GenerateViewConfigJs can generate for 'EnumPropNativeComponent.js'
Failed to find type definition for "ViewProps", please check that you have a valid codegen flow file
24 | return typeAlias.right.typeParameters.params[0].properties;
25 | } catch (e) {
> 26 | throw new Error(
| ^
27 | `Failed to find type definition for "${typeName}", please check that you have a valid codegen flow file`,
28 | );
29 | }

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

[cipolleschi]

Hi @Pranav-yadav! I pointed out some changes to help you mov forward.

Let me know if you need further help here!

[Pranav-yadav]

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

Thanks @cipolleschi. I started again with atomic changes and to make sure everything works as before, eventually I had to move the entire extends.js into props.js.

PS: If we still want to keep extends.js it'll add extra overhead, also eventually we'll need all of it's logic in props.js :)

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,622,804	+0
android	hermes	armeabi-v7a	7,936,158	+0
android	hermes	x86	9,109,453	+0
android	hermes	x86_64	8,964,474	+0
android	jsc	arm64-v8a	9,187,054	+0
android	jsc	armeabi-v7a	8,377,930	+0
android	jsc	x86	9,245,111	+0
android	jsc	x86_64	9,503,774	+0

Base commit: cff4bc8
Branch: main

[Pranav-yadav]

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

The rebase comments didn't work. Strange! 🤔

Btw, looks like you missed the following comment;

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[Pranav-yadav]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

ping @cipolleschi whenever you get time have a look at it.

[Pranav-yadav]

/rebase

Reason: CI seems to be fixed!

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

Hi @Pranav-yadav, you can see the engine we support from the package.json. Apps created from the template have a similar block in their package.json

[Pranav-yadav]

Thanks.
I had checked earlier that the root package.json defines >=16.
Just wasn't sure about the rest of the packages under RN.

[facebook-github-bot]

@cipolleschi merged this pull request in efc6e14.

[Pranav-yadav]

Summary:

[Codegen 102] This PR is subtask of umbrella #34872. It extracts the code to compute the extendsProps and the props properties in Flow in a getProps() -> {extendsProps, props} function into the same index.js file. This will help unifying the buildComponentSchema functions between Flow and TS so we can factor it out in a later step.

Changelog:

[INTERNAL][CHANGED] - merge getExtendsProps & getProps fns into getProps fn - Flow.

Test Plan:

• yarn flow && yarn test react-native-codegen --> should be green.

[Pranav-yadav]

... getProps() -> {extendsProps, props} function into the sameindex.js file.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

[Pranav-yadav]

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

[cipolleschi]

Hi @Pranav-yadav, thanks for taking the time to look into this. You can also look at the twin PR here for more inspiration.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

We can move both to props.js so that the index.js is thinner

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

Yeah, tests are failing. Do they fail locally for you or do they pass locally?
In theory, this is just a move, so they have to keep pass everywhere.
The error says:

GenerateViewConfigJs can generate for 'EnumPropNativeComponent.js'
Failed to find type definition for "ViewProps", please check that you have a valid codegen flow file
24 | return typeAlias.right.typeParameters.params[0].properties;
25 | } catch (e) {
> 26 | throw new Error(
| ^
27 | `Failed to find type definition for "${typeName}", please check that you have a valid codegen flow file`,
28 | );
29 | }

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

[cipolleschi]

Hi @Pranav-yadav! I pointed out some changes to help you mov forward.

Let me know if you need further help here!

[Pranav-yadav]

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

Thanks @cipolleschi. I started again with atomic changes and to make sure everything works as before, eventually I had to move the entire extends.js into props.js.

PS: If we still want to keep extends.js it'll add extra overhead, also eventually we'll need all of it's logic in props.js :)

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,622,804	+0
android	hermes	armeabi-v7a	7,936,158	+0
android	hermes	x86	9,109,453	+0
android	hermes	x86_64	8,964,474	+0
android	jsc	arm64-v8a	9,187,054	+0
android	jsc	armeabi-v7a	8,377,930	+0
android	jsc	x86	9,245,111	+0
android	jsc	x86_64	9,503,774	+0

Base commit: cff4bc8
Branch: main

[Pranav-yadav]

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

The rebase comments didn't work. Strange! 🤔

Btw, looks like you missed the following comment;

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[Pranav-yadav]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

ping @cipolleschi whenever you get time have a look at it.

[Pranav-yadav]

/rebase

Reason: CI seems to be fixed!

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

Hi @Pranav-yadav, you can see the engine we support from the package.json. Apps created from the template have a similar block in their package.json

[Pranav-yadav]

Thanks.
I had checked earlier that the root package.json defines >=16.
Just wasn't sure about the rest of the packages under RN.

[facebook-github-bot]

@cipolleschi merged this pull request in efc6e14.

[Pranav-yadav]

Summary:

[Codegen 102] This PR is subtask of umbrella #34872. It extracts the code to compute the extendsProps and the props properties in Flow in a getProps() -> {extendsProps, props} function into the same index.js file. This will help unifying the buildComponentSchema functions between Flow and TS so we can factor it out in a later step.

Changelog:

[INTERNAL][CHANGED] - merge getExtendsProps & getProps fns into getProps fn - Flow.

Test Plan:

• yarn flow && yarn test react-native-codegen --> should be green.

[Pranav-yadav]

... getProps() -> {extendsProps, props} function into the sameindex.js file.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

[Pranav-yadav]

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

[cipolleschi]

Hi @Pranav-yadav, thanks for taking the time to look into this. You can also look at the twin PR here for more inspiration.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

We can move both to props.js so that the index.js is thinner

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

Yeah, tests are failing. Do they fail locally for you or do they pass locally?
In theory, this is just a move, so they have to keep pass everywhere.
The error says:

GenerateViewConfigJs can generate for 'EnumPropNativeComponent.js'
Failed to find type definition for "ViewProps", please check that you have a valid codegen flow file
24 | return typeAlias.right.typeParameters.params[0].properties;
25 | } catch (e) {
> 26 | throw new Error(
| ^
27 | `Failed to find type definition for "${typeName}", please check that you have a valid codegen flow file`,
28 | );
29 | }

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

[cipolleschi]

Hi @Pranav-yadav! I pointed out some changes to help you mov forward.

Let me know if you need further help here!

[Pranav-yadav]

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

Thanks @cipolleschi. I started again with atomic changes and to make sure everything works as before, eventually I had to move the entire extends.js into props.js.

PS: If we still want to keep extends.js it'll add extra overhead, also eventually we'll need all of it's logic in props.js :)

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,622,804	+0
android	hermes	armeabi-v7a	7,936,158	+0
android	hermes	x86	9,109,453	+0
android	hermes	x86_64	8,964,474	+0
android	jsc	arm64-v8a	9,187,054	+0
android	jsc	armeabi-v7a	8,377,930	+0
android	jsc	x86	9,245,111	+0
android	jsc	x86_64	9,503,774	+0

Base commit: cff4bc8
Branch: main

[Pranav-yadav]

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

The rebase comments didn't work. Strange! 🤔

Btw, looks like you missed the following comment;

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[Pranav-yadav]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

ping @cipolleschi whenever you get time have a look at it.

[Pranav-yadav]

/rebase

Reason: CI seems to be fixed!

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

Hi @Pranav-yadav, you can see the engine we support from the package.json. Apps created from the template have a similar block in their package.json

[Pranav-yadav]

Thanks.
I had checked earlier that the root package.json defines >=16.
Just wasn't sure about the rest of the packages under RN.

[facebook-github-bot]

@cipolleschi merged this pull request in efc6e14.

[Pranav-yadav]

Summary:

[Codegen 102] This PR is subtask of umbrella #34872. It extracts the code to compute the extendsProps and the props properties in Flow in a getProps() -> {extendsProps, props} function into the same index.js file. This will help unifying the buildComponentSchema functions between Flow and TS so we can factor it out in a later step.

Changelog:

[INTERNAL][CHANGED] - merge getExtendsProps & getProps fns into getProps fn - Flow.

Test Plan:

• yarn flow && yarn test react-native-codegen --> should be green.

[Pranav-yadav]

... getProps() -> {extendsProps, props} function into the sameindex.js file.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

[Pranav-yadav]

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

[cipolleschi]

Hi @Pranav-yadav, thanks for taking the time to look into this. You can also look at the twin PR here for more inspiration.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

We can move both to props.js so that the index.js is thinner

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

Yeah, tests are failing. Do they fail locally for you or do they pass locally?
In theory, this is just a move, so they have to keep pass everywhere.
The error says:

GenerateViewConfigJs can generate for 'EnumPropNativeComponent.js'
Failed to find type definition for "ViewProps", please check that you have a valid codegen flow file
24 | return typeAlias.right.typeParameters.params[0].properties;
25 | } catch (e) {
> 26 | throw new Error(
| ^
27 | `Failed to find type definition for "${typeName}", please check that you have a valid codegen flow file`,
28 | );
29 | }

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

[cipolleschi]

Hi @Pranav-yadav! I pointed out some changes to help you mov forward.

Let me know if you need further help here!

[Pranav-yadav]

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

Thanks @cipolleschi. I started again with atomic changes and to make sure everything works as before, eventually I had to move the entire extends.js into props.js.

PS: If we still want to keep extends.js it'll add extra overhead, also eventually we'll need all of it's logic in props.js :)

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,622,804	+0
android	hermes	armeabi-v7a	7,936,158	+0
android	hermes	x86	9,109,453	+0
android	hermes	x86_64	8,964,474	+0
android	jsc	arm64-v8a	9,187,054	+0
android	jsc	armeabi-v7a	8,377,930	+0
android	jsc	x86	9,245,111	+0
android	jsc	x86_64	9,503,774	+0

Base commit: cff4bc8
Branch: main

[Pranav-yadav]

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

The rebase comments didn't work. Strange! 🤔

Btw, looks like you missed the following comment;

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[Pranav-yadav]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

ping @cipolleschi whenever you get time have a look at it.

[Pranav-yadav]

/rebase

Reason: CI seems to be fixed!

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

Hi @Pranav-yadav, you can see the engine we support from the package.json. Apps created from the template have a similar block in their package.json

[Pranav-yadav]

Thanks.
I had checked earlier that the root package.json defines >=16.
Just wasn't sure about the rest of the packages under RN.

[facebook-github-bot]

@cipolleschi merged this pull request in efc6e14.

[Pranav-yadav]

Summary:

[Codegen 102] This PR is subtask of umbrella #34872. It extracts the code to compute the extendsProps and the props properties in Flow in a getProps() -> {extendsProps, props} function into the same index.js file. This will help unifying the buildComponentSchema functions between Flow and TS so we can factor it out in a later step.

Changelog:

[INTERNAL][CHANGED] - merge getExtendsProps & getProps fns into getProps fn - Flow.

Test Plan:

• yarn flow && yarn test react-native-codegen --> should be green.

[Pranav-yadav]

... getProps() -> {extendsProps, props} function into the sameindex.js file.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

[Pranav-yadav]

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

[cipolleschi]

Hi @Pranav-yadav, thanks for taking the time to look into this. You can also look at the twin PR here for more inspiration.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

We can move both to props.js so that the index.js is thinner

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

Yeah, tests are failing. Do they fail locally for you or do they pass locally?
In theory, this is just a move, so they have to keep pass everywhere.
The error says:

GenerateViewConfigJs can generate for 'EnumPropNativeComponent.js'
Failed to find type definition for "ViewProps", please check that you have a valid codegen flow file
24 | return typeAlias.right.typeParameters.params[0].properties;
25 | } catch (e) {
> 26 | throw new Error(
| ^
27 | `Failed to find type definition for "${typeName}", please check that you have a valid codegen flow file`,
28 | );
29 | }

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

[cipolleschi]

Hi @Pranav-yadav! I pointed out some changes to help you mov forward.

Let me know if you need further help here!

[Pranav-yadav]

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

Thanks @cipolleschi. I started again with atomic changes and to make sure everything works as before, eventually I had to move the entire extends.js into props.js.

PS: If we still want to keep extends.js it'll add extra overhead, also eventually we'll need all of it's logic in props.js :)

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,622,804	+0
android	hermes	armeabi-v7a	7,936,158	+0
android	hermes	x86	9,109,453	+0
android	hermes	x86_64	8,964,474	+0
android	jsc	arm64-v8a	9,187,054	+0
android	jsc	armeabi-v7a	8,377,930	+0
android	jsc	x86	9,245,111	+0
android	jsc	x86_64	9,503,774	+0

Base commit: cff4bc8
Branch: main

[Pranav-yadav]

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

The rebase comments didn't work. Strange! 🤔

Btw, looks like you missed the following comment;

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[Pranav-yadav]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

ping @cipolleschi whenever you get time have a look at it.

[Pranav-yadav]

/rebase

Reason: CI seems to be fixed!

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

Hi @Pranav-yadav, you can see the engine we support from the package.json. Apps created from the template have a similar block in their package.json

[Pranav-yadav]

Thanks.
I had checked earlier that the root package.json defines >=16.
Just wasn't sure about the rest of the packages under RN.

[facebook-github-bot]

@cipolleschi merged this pull request in efc6e14.

[Pranav-yadav]

Summary:

[Codegen 102] This PR is subtask of umbrella #34872. It extracts the code to compute the extendsProps and the props properties in Flow in a getProps() -> {extendsProps, props} function into the same index.js file. This will help unifying the buildComponentSchema functions between Flow and TS so we can factor it out in a later step.

Changelog:

[INTERNAL][CHANGED] - merge getExtendsProps & getProps fns into getProps fn - Flow.

Test Plan:

• yarn flow && yarn test react-native-codegen --> should be green.

[Pranav-yadav]

... getProps() -> {extendsProps, props} function into the sameindex.js file.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

[Pranav-yadav]

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

[cipolleschi]

Hi @Pranav-yadav, thanks for taking the time to look into this. You can also look at the twin PR here for more inspiration.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

We can move both to props.js so that the index.js is thinner

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

Yeah, tests are failing. Do they fail locally for you or do they pass locally?
In theory, this is just a move, so they have to keep pass everywhere.
The error says:

GenerateViewConfigJs can generate for 'EnumPropNativeComponent.js'
Failed to find type definition for "ViewProps", please check that you have a valid codegen flow file
24 | return typeAlias.right.typeParameters.params[0].properties;
25 | } catch (e) {
> 26 | throw new Error(
| ^
27 | `Failed to find type definition for "${typeName}", please check that you have a valid codegen flow file`,
28 | );
29 | }

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

[cipolleschi]

Hi @Pranav-yadav! I pointed out some changes to help you mov forward.

Let me know if you need further help here!

[Pranav-yadav]

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

Thanks @cipolleschi. I started again with atomic changes and to make sure everything works as before, eventually I had to move the entire extends.js into props.js.

PS: If we still want to keep extends.js it'll add extra overhead, also eventually we'll need all of it's logic in props.js :)

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,622,804	+0
android	hermes	armeabi-v7a	7,936,158	+0
android	hermes	x86	9,109,453	+0
android	hermes	x86_64	8,964,474	+0
android	jsc	arm64-v8a	9,187,054	+0
android	jsc	armeabi-v7a	8,377,930	+0
android	jsc	x86	9,245,111	+0
android	jsc	x86_64	9,503,774	+0

Base commit: cff4bc8
Branch: main

[Pranav-yadav]

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

The rebase comments didn't work. Strange! 🤔

Btw, looks like you missed the following comment;

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[Pranav-yadav]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

ping @cipolleschi whenever you get time have a look at it.

[Pranav-yadav]

/rebase

Reason: CI seems to be fixed!

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

Hi @Pranav-yadav, you can see the engine we support from the package.json. Apps created from the template have a similar block in their package.json

[Pranav-yadav]

Thanks.
I had checked earlier that the root package.json defines >=16.
Just wasn't sure about the rest of the packages under RN.

[facebook-github-bot]

@cipolleschi merged this pull request in efc6e14.

[Pranav-yadav]

Summary:

[Codegen 102] This PR is subtask of umbrella #34872. It extracts the code to compute the extendsProps and the props properties in Flow in a getProps() -> {extendsProps, props} function into the same index.js file. This will help unifying the buildComponentSchema functions between Flow and TS so we can factor it out in a later step.

Changelog:

[INTERNAL][CHANGED] - merge getExtendsProps & getProps fns into getProps fn - Flow.

Test Plan:

• yarn flow && yarn test react-native-codegen --> should be green.

[Pranav-yadav]

... getProps() -> {extendsProps, props} function into the sameindex.js file.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

[Pranav-yadav]

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

[cipolleschi]

Hi @Pranav-yadav, thanks for taking the time to look into this. You can also look at the twin PR here for more inspiration.

@cipolleschi do we want to extract final getProps() into index.js or props.js?
Since, extracting into index.js would require moving other helper functions from both extends.js and props.js in it.

We can move both to props.js so that the index.js is thinner

Also, both flow and lint are green yet the tests seem to fail. Am I missing something ?

Yeah, tests are failing. Do they fail locally for you or do they pass locally?
In theory, this is just a move, so they have to keep pass everywhere.
The error says:

GenerateViewConfigJs can generate for 'EnumPropNativeComponent.js'
Failed to find type definition for "ViewProps", please check that you have a valid codegen flow file
24 | return typeAlias.right.typeParameters.params[0].properties;
25 | } catch (e) {
> 26 | throw new Error(
| ^
27 | `Failed to find type definition for "${typeName}", please check that you have a valid codegen flow file`,
28 | );
29 | }

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

[cipolleschi]

Hi @Pranav-yadav! I pointed out some changes to help you mov forward.

Let me know if you need further help here!

[Pranav-yadav]

So, I think you moved something that you shouldn't and now it fails to find the definition.

I'll try to rollback the changes and I'll try to apply them one at the time to see what broke the system.

Thanks @cipolleschi. I started again with atomic changes and to make sure everything works as before, eventually I had to move the entire extends.js into props.js.

PS: If we still want to keep extends.js it'll add extra overhead, also eventually we'll need all of it's logic in props.js :)

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,622,804	+0
android	hermes	armeabi-v7a	7,936,158	+0
android	hermes	x86	9,109,453	+0
android	hermes	x86_64	8,964,474	+0
android	jsc	arm64-v8a	9,187,054	+0
android	jsc	armeabi-v7a	8,377,930	+0
android	jsc	x86	9,245,111	+0
android	jsc	x86_64	9,503,774	+0

Base commit: cff4bc8
Branch: main

[Pranav-yadav]

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

The rebase comments didn't work. Strange! 🤔

Btw, looks like you missed the following comment;

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?

Edit: Actually, the main question is about ECMAScript versions compatibility :)

[Pranav-yadav]

/rebase

[Pranav-yadav]

/rebase

[Pranav-yadav]

ping @cipolleschi whenever you get time have a look at it.

[Pranav-yadav]

/rebase

Reason: CI seems to be fixed!

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

Hi @Pranav-yadav, you can see the engine we support from the package.json. Apps created from the template have a similar block in their package.json

[Pranav-yadav]

Thanks.
I had checked earlier that the root package.json defines >=16.
Just wasn't sure about the rest of the packages under RN.

[facebook-github-bot]

@cipolleschi merged this pull request in efc6e14.