Extract MisnamedModuleInterfaceParserError from Flow and Typescript into error-utils.js #34916

mohitcharkha · 2022-10-10T06:53:29Z

Summary

This PR is part of #34872
This PR extracts MisnamedModuleFlowInterfaceParserError exception to a separate function inside an error-utils.js file

Changelog

[Internal] [Changed] - Extract MisnamedModuleInterfaceParserError to a seperate function inside error-utils.js

Test Plan

yarn jest react-native-codegen
Added unit case in error-utils-test.js file

…nto error-utils.js

analysis-bot · 2022-10-10T07:20:14Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,778,150	+0
android	hermes	armeabi-v7a	7,179,206	+0
android	hermes	x86	8,091,069	+0
android	hermes	x86_64	8,062,841	+0
android	jsc	arm64-v8a	9,637,352	+0
android	jsc	armeabi-v7a	8,401,647	+0
android	jsc	x86	9,586,544	+0
android	jsc	x86_64	10,179,709	+0

Base commit: 74fff88
Branch: main

analysis-bot · 2022-10-10T07:59:51Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 74fff88
Branch: main

AntoineDoubovetzky · 2022-10-10T08:07:18Z

@mohitcharkha I don't think the goal of this task was to extract the error itself, it is done for all errors here: #34896

The goal is to extract the logic that makes the check and throw if necessary (see #34872 (comment))

cipolleschi

Thanks a lot for taking this. I left a comment in the code with a change that will make the PR represent better the task!

Sorry if the task description was not clear in the first place! 🙏

cipolleschi · 2022-10-10T08:15:50Z

packages/react-native-codegen/src/parsers/flow/modules/index.js

  if (moduleSpec.id.name !== 'Spec') {
-    throw new MisnamedModuleFlowInterfaceParserError(
+    throw new MisnamedModuleInterfaceParserError(
      hasteModuleName,
      moduleSpec.id,
+      'Flow',
    );


The tasks was slightly different: this PR export just the type from the index.js to the error-utils.
However, this step was responsibility of #34896 (which this PR should depend on).

What I was expecting here, was a function like:

function throwIfModuleInterfaceIsMisnamed(nativeModuleName: string, module: ASTNode, parserType: ParserType) { if (moduleSpec.id.name !== 'Spec') { throw new error } }

That will replace these 6 lines in the flow/modules/index.js and in the typescript/modules/index.js.

In this way:

if we need to update the logic that throws the error, we do it in a single place

the parser code is more readable and self documenting.

Could you update this with this suggestion?

Thank you so much! 🙏

cipolleschi · 2022-10-10T08:16:12Z

packages/react-native-codegen/src/parsers/typescript/modules/index.js

  if (moduleSpec.id.name !== 'Spec') {
-    throw new MisnamedModuleTypeScriptInterfaceParserError(
+    throw new MisnamedModuleInterfaceParserError(
      hasteModuleName,
      moduleSpec.id,
+      'TypeScript',
    );
  }


Same change described above! 😉

cipolleschi · 2022-10-10T08:16:41Z

packages/react-native-codegen/src/parsers/error-utils.js

+
+const {ParserError} = require('./errors');
+
+type ParserTypes = 'Flow' | 'TypeScript';


I love this! 😄 I'm moving the suggestion to the other PR (#34896).

cipolleschi · 2022-10-10T09:03:07Z

Also, given that we are introducing a new function, it would be great to have a unit test for it!

…rError

facebook-github-bot · 2022-10-10T11:01:59Z

Hi @mohitcharkha!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2022-10-10T11:12:19Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

cipolleschi

Looks good! Thank you so much for taking this task!

…rError

facebook-github-bot · 2022-10-10T13:38:30Z

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

packages/react-native-codegen/src/parsers/__tests__/error-utils-test.js

…rError

…faceParserError

cipolleschi · 2022-10-14T15:24:28Z

@mohitcharkha could you rebase and resolve the conflicts, please?

…oid function (facebook#34932) Summary: Part of facebook#34872 This PR: - extracts the content of the case 'VoidTypeAnnotation' ([Flow](https://github.com/facebook/react-native/blob/b444f0e44e0d8670139acea5f14c2de32c5e2ddc/packages/react-native-codegen/src/parsers/flow/modules/index.js#L375-L377), [TypeScript](https://github.com/facebook/react-native/blob/00b795642a6562fb52d6df12e367b84674994623/packages/react-native-codegen/src/parsers/typescript/modules/index.js#L410-L412)) into a single emitVoid function in the parsers-primitives.js file. Use the new function in the parsers. - unit tests emitVoid function ## Changelog [Internal] [Changed] - Extract contents of the case 'VoidTypeAnnotation' into a single emitVoid function Pull Request resolved: facebook#34932 Test Plan: ` yarn jest react-native-codegen` <img width="957" alt="image" src="https://user-images.githubusercontent.com/19575877/194931977-d5cfc5f5-c9db-498d-9e5c-ae40a38d3623.png"> Reviewed By: cipolleschi Differential Revision: D40239730 Pulled By: rshest fbshipit-source-id: 16a9555223cacbb3b9916fd469bd63f83db33f18

…unction (facebook#34936) Summary: This PR extracts the content of the codegen case `'Stringish'` into a single `emitStringish` function inside the `parsers-primitives.js` file and uses it in both Flow and TypeScript parsers as requested on facebook#34872. This also adds unit tests to the new `emitStringish` function. ## Changelog [Internal] [Changed] - Extract the content of the case 'Stringish' into a single emitStringish function Pull Request resolved: facebook#34936 Test Plan: Run `yarn jest react-native-codegen` and ensure CI is green ![image](https://user-images.githubusercontent.com/11707729/194987664-b588b82b-a9e0-49a9-a3cc-a03cb0a230e6.png) Reviewed By: cipolleschi Differential Revision: D40255921 Pulled By: rshest fbshipit-source-id: 9c08f81f12c93995bb6ba032fabcd6451b8dc7c1

Summary: I'm cleaning up the Buck setup in the New App template. It hasn't been touched since 2018 and is currently not working, but will end up being propagated in user space. This is also affecting the Gradle setup as there is a need to have an extra task just for Buck. I doubt anyone is using this at this stage. We can get back to it or have a dedicated guide for users that are willing to use it. ## Changelog [Android] [Remove] - Cleanup Buck usages from New App Template Pull Request resolved: facebook#34881 Test Plan: Testing was missing on this API so there is nothing to test here Reviewed By: cipolleschi Differential Revision: D40142064 Pulled By: cortinico fbshipit-source-id: 6c65e1030bf3fe1603bfb7d486f8f3fd7d254126

Summary: Changelog [General][Internal] - Make react-native-codegen work on Windows within BUCK This makes react-native code-gen work on Windows - by basically filling in some necessary windows commands for the existing bash/shell ones Reviewed By: cortinico, cipolleschi Differential Revision: D40268750 fbshipit-source-id: 0db7597999ec8108babf56abed436f6dc7789cce

Summary: Pull Request resolved: facebook#34943 The RCTAppDelegate class is the new way we have to encapsulate logic we don't want to leak to the average user. However, some advanced users, like Expo users, may need more options to customize their setup. This Diff provides more methods to extend the AppDelegate. ## Changelog [iOS][Added] - Add more extension points for RCTAppDelegate Reviewed By: cortinico Differential Revision: D40262513 fbshipit-source-id: 9365a51d938a586b1508233bfa63693cf9aebf7a

Summary: I'm suppressing this deprecation on `reactRoot` as it's unnecessary. It creates noise and the usage is actually legit. Changelog: [Internal] [Changed] - Suppress deprecation on reactRoot passed as deprecatedReactRoot Reviewed By: cipolleschi Differential Revision: D40139555 fbshipit-source-id: f3755f020ab1dceb17d0fdd18c6c8e374c87c60c

Reviewed By: jkeljo Differential Revision: D40377133 fbshipit-source-id: d029452e34edecdf968aa60fb8a3a062bca60d92

Summary: changelog: Fix vertical text alignment in new architecture when line height is not 0. Reviewed By: javache Differential Revision: D40346225 fbshipit-source-id: f6282cb8df80e9a543e5602fddcca1a1a82377e3

Differential Revision: D40374535 (facebook@5359f1d) Original commit changeset: 92788a21a4f2 Original Phabricator Diff: D40374535 (facebook@5359f1d) fbshipit-source-id: a0115dc24590bc50fa1aee2770dbb13b2edaf580

…dler Summary: Changelog: [Internal] Code clarity: Use enum to clarify the values parsed by JsErrorHandler Create a enum `JSErrorHandlerKey` to define the keys for the values that goes into each stack frame. Next up -- using this in the Bridge! Reviewed By: sammy-SC Differential Revision: D40326127 fbshipit-source-id: e416814fbcaa922afa92d9baf79eaa898ff979f0

Summary: There's no need for the 3 type arguments here. Flow will infer a union already if multiple properties are provided. Worse, by not providing these properties these tvars end up with no bounds, which can cause downstream constraints to stall. All of the suppresisons added here are for legitimate errors that were uncovered by consolidating to one type argument. Changelog: [internal] Reviewed By: SamChou19815 Differential Revision: D40355811 fbshipit-source-id: 088fd087017a6082c793ef00c8810a81b39fb9fb

Summary: The jsi library itself is `jsi/jsi.{h,cpp}`. JSIDynamic provides support for converting between folly::dynamic and jsi::value, independent of the jsi library. Changelog: [iOS][Changed] Moved JSIDynamic out of React-jsi and into React-jsidynamic Reviewed By: cipolleschi, dmytrorykun Differential Revision: D40334023 fbshipit-source-id: d2c69e7afb7f43f93080301b88c81e1fa46279d7

Summary: Changelog: [Internal] Reviewed By: jbrown215 Differential Revision: D40393734 fbshipit-source-id: cc7a5e1fd28512c8141a0f705580a447efcd5575

Summary: This diff adds defaults to Jest.fn's type params. An upcoming change to flow will be more restrictive about how we implicitly instantiation type arguments. Since the argument to jest.fn is often omitted, this signature is the single biggest offender of our new rules. To reduce the error diff, I'm adding reasonable defaults so that the mocked function can still behave like a function without adding extra flow errors where there were none before. Changelog: [Internal] Reviewed By: SamChou19815 Differential Revision: D40394215 fbshipit-source-id: 2df596b6ba8b54680694e08c432a1e7c94dc2df0

Summary: Changelog: [General][Added] - Highlight elements on hover while mouse down for React DevTools element inspection. Since there is probably no mouse hover events for RN, this diff implements something that works similar like hover for RN: user keeps the mouse down and moves the cursor around, and the elements under the mouse is highlighted just like Web. Reviewed By: lunaruan Differential Revision: D40369733 fbshipit-source-id: ef223ee0f31f4e0372674fc39dd13bad8c15aa92

Summary: Changelog: [Internal] Reorder and prefix things with wpt for convenience Open to other suggestions Reviewed By: mdvacca Differential Revision: D40247488 fbshipit-source-id: 4046040d5904b324a9a84dc47f4de858456e7bbc

Summary: Changelog: [Internal] - Add an option to only see failed tests for PointerEvents web platform tests Reviewed By: mdvacca Differential Revision: D40281369 fbshipit-source-id: 638879b39ca66c11e722e6140c631dacfc98ee34

Summary: Changelog: [Internal] Add more properties to PointerEvent, ensure all web platform tests are passing Reviewed By: mdvacca Differential Revision: D40314330 fbshipit-source-id: 071683f26f5a1e17d7ea49ac022c0ca23b6f9947

Summary: Changelog: [Internal] Reviewed By: jbrown215 Differential Revision: D40408230 fbshipit-source-id: 15b3190fccc2972075823947d413c71ec3eff7f0

github-actions · 2022-10-17T06:18:31Z

packages/react-native-codegen/src/parsers/__tests__/error-utils-test.js

+  });
+});
+
+describe('throwIfMoreThanOneModuleRegistryCalls', () => {


⚠️ packages/react-native-codegen/src/parsers/tests/error-utils-test.js line 470 – Describe block title is used multiple times in the same describe block. (jest/no-identical-title)

…rError

packages/react-native-codegen/src/parsers/typescript/modules/index.js

…ndex.js

packages/react-native-codegen/src/parsers/flow/modules/index.js

cipolleschi · 2022-10-17T07:26:43Z

/rebase

facebook-github-bot · 2022-10-17T12:09:01Z

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

react-native-bot · 2022-10-17T13:55:05Z

This pull request was successfully merged by @mohitcharkha in 9fb3700.

^{When will my fix make it into a release? | Upcoming Releases}

…nto error-utils.js (facebook#34916) Summary: This PR is part of facebook#34872 This PR extracts MisnamedModuleFlowInterfaceParserError exception to a separate function inside an error-utils.js file ## Changelog  [Internal] [Changed] - Extract MisnamedModuleInterfaceParserError to a seperate function inside error-utils.js Pull Request resolved: facebook#34916 Test Plan: yarn jest react-native-codegen Added unit case in `error-utils-test.js` file <img width="980" alt="Extract MisnamedModuleInterfaceParserError test Screenshot" src="https://user-images.githubusercontent.com/86604753/194853899-22c1ce05-fe55-4102-a83b-15c707a20000.png"> Reviewed By: cipolleschi Differential Revision: D40226541 Pulled By: motiz88 fbshipit-source-id: 6698ceff192c592383aa3419ac31de524c605919

Extract MisnamedModuleInterfaceParserError from Flow and Typescript i…

8b106eb

…nto error-utils.js

cipolleschi added the hacktoberfest-accepted label Oct 10, 2022

cipolleschi requested changes Oct 10, 2022

View reviewed changes

cipolleschi mentioned this pull request Oct 10, 2022

[Codegen]: Extract module/errors into a shared file #34896

Closed

mohitcharkha added 2 commits October 10, 2022 15:44

Merge branch 'main' into extract-codegen-MisnamedModuleInterfaceParse…

9fb6b77

…rError

Added unit test cases and other changes

2f33dea

Updated export and import of function

2e203bc

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 10, 2022

cipolleschi approved these changes Oct 10, 2022

View reviewed changes

Merge branch 'main' into extract-codegen-MisnamedModuleInterfaceParse…

dce9e36

…rError

cipolleschi requested changes Oct 10, 2022

View reviewed changes

packages/react-native-codegen/src/parsers/__tests__/error-utils-test.js Show resolved Hide resolved

Enabled flow in error-utils-test.js file

0bc8df2

mohitcharkha requested a review from cipolleschi October 10, 2022 14:04

cipolleschi approved these changes Oct 10, 2022

View reviewed changes

mohitcharkha and others added 2 commits October 11, 2022 18:07

Merge branch 'main' into extract-codegen-MisnamedModuleInterfaceParse…

dc7f588

…rError

Merge branch 'facebook:main' into extract-codegen-MisnamedModuleInter…

1a5c426

…faceParserError

youedd and others added 6 commits October 17, 2022 11:37

generatedunixname89002005287564 and others added 13 commits October 17, 2022 11:47

supermodule:xplat/default/public.react_native.playground

3d8e0bd

Reviewed By: jkeljo Differential Revision: D40377133 fbshipit-source-id: d029452e34edecdf968aa60fb8a3a062bca60d92

Center text if line height isn't 0

98467e1

Summary: changelog: Fix vertical text alignment in new architecture when line height is not 0. Reviewed By: javache Differential Revision: D40346225 fbshipit-source-id: f6282cb8df80e9a543e5602fddcca1a1a82377e3

Revert D40374535: Deploy 0.190.0 to xplat

4f8f9c8

Differential Revision: D40374535 (facebook@5359f1d) Original commit changeset: 92788a21a4f2 Original Phabricator Diff: D40374535 (facebook@5359f1d) fbshipit-source-id: a0115dc24590bc50fa1aee2770dbb13b2edaf580

Deploy 0.190.0 to xplat

68a5917

Summary: Changelog: [Internal] Reviewed By: jbrown215 Differential Revision: D40393734 fbshipit-source-id: cc7a5e1fd28512c8141a0f705580a447efcd5575

Prefix web platform tests with wpt

9a08ab5

Summary: Changelog: [Internal] Reorder and prefix things with wpt for convenience Open to other suggestions Reviewed By: mdvacca Differential Revision: D40247488 fbshipit-source-id: 4046040d5904b324a9a84dc47f4de858456e7bbc

Add option to only see failed tests

02b42f1

Summary: Changelog: [Internal] - Add an option to only see failed tests for PointerEvents web platform tests Reviewed By: mdvacca Differential Revision: D40281369 fbshipit-source-id: 638879b39ca66c11e722e6140c631dacfc98ee34

Add width, height, detail props

3e29a03

Summary: Changelog: [Internal] Add more properties to PointerEvent, ensure all web platform tests are passing Reviewed By: mdvacca Differential Revision: D40314330 fbshipit-source-id: 071683f26f5a1e17d7ea49ac022c0ca23b6f9947

Fix or expose previously hidden type errors to prepare for LTI mode

c419939

Summary: Changelog: [Internal] Reviewed By: jbrown215 Differential Revision: D40408230 fbshipit-source-id: 15b3190fccc2972075823947d413c71ec3eff7f0

mohitcharkha requested a review from hramos as a code owner October 17, 2022 06:17

github-actions bot reviewed Oct 17, 2022

View reviewed changes

Merge branch 'main' into extract-codegen-MisnamedModuleInterfaceParse…

0a2c4ab

…rError

mohitcharkha commented Oct 17, 2022

View reviewed changes

packages/react-native-codegen/src/parsers/typescript/modules/index.js Outdated Show resolved Hide resolved

Update packages/react-native-codegen/src/parsers/typescript/modules/i…

a2e0039

…ndex.js

mohitcharkha commented Oct 17, 2022

View reviewed changes

packages/react-native-codegen/src/parsers/flow/modules/index.js Outdated Show resolved Hide resolved

Update packages/react-native-codegen/src/parsers/flow/modules/index.js

43fba6a

mohitcharkha requested review from cipolleschi and removed request for hramos October 17, 2022 06:29

cipolleschi approved these changes Oct 17, 2022

View reviewed changes

facebook-github-bot closed this in 9fb3700 Oct 17, 2022

react-native-bot added the Merged This PR has been merged. label Oct 17, 2022


		const {ParserError} = require('./errors');

		type ParserTypes = 'Flow' \| 'TypeScript';

Extract MisnamedModuleInterfaceParserError from Flow and Typescript into error-utils.js #34916

Extract MisnamedModuleInterfaceParserError from Flow and Typescript into error-utils.js #34916

Uh oh!

Conversation

mohitcharkha commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog

Test Plan

Uh oh!

analysis-bot commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AntoineDoubovetzky commented Oct 10, 2022

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

cipolleschi Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

cipolleschi Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

cipolleschi Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

cipolleschi commented Oct 10, 2022

Uh oh!

facebook-github-bot commented Oct 10, 2022

Action Required

Process

Uh oh!

facebook-github-bot commented Oct 10, 2022

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 10, 2022

Uh oh!

Uh oh!

cipolleschi commented Oct 14, 2022

Uh oh!

github-actions bot Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cipolleschi commented Oct 17, 2022

Uh oh!

facebook-github-bot commented Oct 17, 2022

Uh oh!

react-native-bot commented Oct 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

mohitcharkha commented Oct 10, 2022 •

edited

Loading

analysis-bot commented Oct 10, 2022 •

edited

Loading

analysis-bot commented Oct 10, 2022 •

edited

Loading