Skip to content

nit: fix typo in new architecture docs#3261

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.

Sign up for GitHub

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

Merged
cipolleschi merged 4 commits into from
Aug 19, 2022
+3 −4
Merged

nit: fix typo in new architecture docs #3261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
275730a
fix typo
intergalacticspacehighway Aug 15, 2022
c2eefd1
fix: typescript example
intergalacticspacehighway Aug 15, 2022
c8ee22f
fix: lint
intergalacticspacehighway Aug 15, 2022
68f0699
make text optional
intergalacticspacehighway Aug 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/the-new-architecture/pillars-codegen.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -114,7 +114,7 @@ In the example above, there are both a TurboModule and a set of Fabric Component

Each TurboModule's folder contains two files: an interface file and an implementation file.

The interface files have the same name of the TurboModule and they contain methods to initialize their the JSI interface.
The interface files have the same name of the TurboModule and they contain methods to initialize the JSI interface.

The implementation files, instead, have the `-generated` suffix and they contains the logic to invoke the native methods from JS and viceversa.

Expand DownExpand Up@@ -218,7 +218,7 @@ Notice that both TurboModules and Fabric Components comes with two build file de

### TurboModule

The **Codegen** generates a Java abstract class in the `java` package which the same name of the TurboModule. This abstract class has to be implemented by the JNI C++ implementation.
The **Codegen** generates a Java abstract class in the `java` package with the same name of the TurboModule. This abstract class has to be implemented by the JNI C++ implementation.

Then, it generates the C++ files in the `jni` folder. They follow the same iOS convention: there is an interface called `MyTurbomodule.h` and an implementation file called `MyTurbomodule-generated.cpp`. The former is an interface that allows React Natvie to initialize the JSI interface for the TurboModule. The latter is the implementation file which contains the logic to invoke the native method from JS and viceversa.

Expand Down
3 changes: 1 addition & 2 deletions docs/the-new-architecture/pillars-fabric-components.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -96,8 +96,7 @@ import type { HostComponent } from 'react-native';
import codegenNativeComponent from 'react-native/Libraries/Utilities/codegenNativeComponent';

export interface NativeProps extends ViewProps {
...ViewProps,

@intergalacticspacehighwayintergalacticspacehighwayAug 15, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think this is not valid in typescript, extending the interface with ViewProps should be enough I guess. Let me know if I am wrong though!

@SimekSimekAug 15, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Agree, this also probably should be type not interface too, as well text should be marked as optional and then undefined can be removed from the type.

Let's see what's others opinion, CC @cipolleschi, @cortinico

@intergalacticspacehighwayintergalacticspacehighwayAug 15, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

yeah, thought it might be required for codegen. I verified that codegen was working after removing the spread syntax.

@intergalacticspacehighwayintergalacticspacehighwayAug 15, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

also, this felt a bit weird. import type {ViewProps} from 'ViewPropTypes';
Thought it should be import type {ViewProps} from 'react-native/Libraries/Components/View/ViewPropTypes'; instead but typescript package is not yet upgraded, so not sure.

@cipolleschicipolleschiAug 17, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, thanks for the comments!

I'm not a TypeScript expert, but I will do some checks.

Could you update the PR making the NativeProps a type and the text property optional?

@intergalacticspacehighwayintergalacticspacehighwayAug 17, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@cipolleschi thanks for checking. I could make those changes but I think current codegen requires component spec to be an interface.

I get an error while performing codegen with below types

// flow equivalent in typescript but uses type instead of an interface. typeNativeProps=Readonly<{text?: string}>&ViewProps

I think @Titozzz who recently did PR in webview is using interface due to same reason.

cc @acoates-ms@ZihanChen-MSFT

text: string | null | undefined,
text?: string;

@intergalacticspacehighwayintergalacticspacehighwayAug 17, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@cipolleschi made the text property optional, verified that codegen works with it

// add other props here
}

Expand Down