[Codegen 104] Add `getResolvedTypeAnnotation` to Parsers #37373

siddarthkay · 2023-05-11T07:22:09Z

Summary:

[Codegen 104] This PR introduces getResolvedTypeAnnotation to the Parser class and implements this function in Typescript and Flow Parsers.

We also get rid of usages resolveTypeAnnotation from :

packages/react-native-codegen/src/parsers/typescript/utils.js
packages/react-native-codegen/src/parsers/flow/utils.js

and replace it with parsers.getResolvedTypeAnnotation as requested on #34872

Changelog:

[Internal] [Changed] - Add getResolvedTypeAnnotation to Parsers and update usages.

Test Plan:

Run yarn jest react-native-codegen and ensure CI is green

analysis-bot · 2023-05-11T07:47:04Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,732,592	-1
android	hermes	armeabi-v7a	8,043,922	-3
android	hermes	x86	9,221,937	-3
android	hermes	x86_64	9,074,915	+3
android	jsc	arm64-v8a	9,297,784	-2
android	jsc	armeabi-v7a	8,486,550	-2
android	jsc	x86	9,358,531	-1
android	jsc	x86_64	9,615,109	-4

Base commit: d8ced6f
Branch: main

siddarthkay · 2023-05-11T10:30:35Z

Tests fail with :

TypeError: translateTypeAnnotation is not a function
Occurred while linting /home/circleci/react-native/packages/react-native/Libraries/
ActionSheetIOS/NativeActionSheetManager.js:11
Rule: "@react-native/specs/react-native-modules"
    at translateFunctionTypeAnnotation (/home/circleci/react-native/packages/
    react-native-codegen/src/parsers/parsers-commons.js:281:7)
    at buildPropertySchema (/home/circleci/react-native/packages/react-native-codegen/
    src/parsers/parsers-commons.js:356:7)
    at /home/circleci/react-native/packages/react-native-codegen/src/parsers/
    parsers-commons.js:629:24
    at guard (/home/circleci/react-native/packages/react-native-codegen/src/parsers/
    utils.js:48:14)
    at /home/circleci/react-native/packages/react-native-codegen/src/parsers/
    parsers-commons.js:626:14
    at Array.map (<anonymous>)
    at buildModuleSchema (/home/circleci/react-native/packages/react-native-codegen/src/
    parsers/parsers-commons.js:618:6)
    at /home/circleci/react-native/packages/eslint-plugin-specs/
    react-native-modules.js:162:9
    at guard (/home/circleci/react-native/packages/react-native-codegen/src/
    parsers/utils.js:48:14)
    at Program:exit (/home/circleci/react-native/packages/eslint-plugin-specs/
    react-native-modules.js:161:7)
    at ruleErrorHandler (/home/circleci/react-native/node_modules/eslint/lib/linter/
    linter.js:1049:28)
    at /home/circleci/react-native/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/home/circleci/react-native/node_modules/eslint/lib/linter/
    safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/home/circleci/react-native/node_modules/
    eslint/lib/linter/node-event-generator.js:297:26)
    at NodeEventGenerator.applySelectors (/home/circleci/react-native/node_modules/
    eslint/lib/linter/node-event-generator.js:326:22)
    at NodeEventGenerator.leaveNode (/home/circleci/react-native/node_modules/eslint/
    lib/linter/node-event-generator.js:349:14)
    at CodePathAnalyzer.leaveNode (/home/circleci/react-native/node_modules/eslint/lib/
    linter/code-path-analysis/code-path-analyzer.js:816:23)
    at /home/circleci/react-native/node_modules/eslint/lib/linter/linter.js:1086:32
    at Array.forEach (<anonymous>)
    at runRules (/home/circleci/react-native/node_modules/eslint/lib/linter/linter.js:1079:15)
    at Linter._verifyWithoutProcessors (/home/circleci/react-native/node_modules/eslint/
    lib/linter/linter.js:1328:31)
    at Linter._verifyWithoutProcessors (/home/circleci/react-native/node_modules/
    eslint-plugin-eslint-comments/lib/utils/patch.js:181:42)
    at Linter._verifyWithConfigArray (/home/circleci/react-native/node_modules/eslint/lib/
    linter/linter.js:1703:21)
    at Linter.verify (/home/circleci/react-native/node_modules/eslint/lib/linter/linter.js:1410:65)
    at Linter.verifyAndFix (/home/circleci/react-native/node_modules/eslint/lib/linter/linter.js:1962:29)
    at verifyText (/home/circleci/react-native/node_modules/eslint/lib/cli-engine/cli-engine.js:245:48)
    at CLIEngine.executeOnFiles (/home/circleci/react-native/node_modules/eslint/lib/
    cli-engine/cli-engine.js:823:28)
    at ESLint.lintFiles (/home/circleci/react-native/node_modules/eslint/lib/eslint/eslint.js:551:23)
    at Object.execute (/home/circleci/react-native/node_modules/eslint/lib/cli.js:417:36)
    at main (/home/circleci/react-native/node_modules/eslint/bin/eslint.js:135:24)
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Failed to run eslint.
Finished.
/home/circleci/react-native/scripts/run-ci-javascript-tests.js:40
    throw Error(exitCode);
    ^

Error: 1
    at Object.<anonymous> (/home/circleci/react-native/scripts/run-ci-javascript-tests.js:40:11)
    at Module._compile (node:internal/modules/cjs/loader:1159:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1213:10)
    at Module.load (node:internal/modules/cjs/loader:1037:32)
    at Module._load (node:internal/modules/cjs/loader:878:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:23:47

When i look at usage of translateTypeAnnotation it does seem to be heavily used in packages/react-native-codegen/src/parsers/parsers-commons.js.
However, translateTypeAnnotation is never imported but passed through many of the functions inside parsers-commons.js

Seems similar to the cyclic dependency described over here : #37043 (comment)

siddarthkay · 2023-05-11T10:48:59Z

translateTypeAnnotation is used in the following functions :

parseObjectProperty
Here translateTypeAnnotation is passed onto unwrapNullable.
translateFunctionTypeAnnotation
Here translateTypeAnnotation is called.
buildPropertySchema
This function calls translateFunctionTypeAnnotation and passes translateTypeAnnotation as a
parameter.
buildSchemaFromConfigType
This function calls buildModuleSchema and passes translateTypeAnnotation as a
parameter.
buildSchema
This function calls buildModuleSchema and passes translateTypeAnnotation as a
parameter.
buildModuleSchema
This function calls buildPropertySchema and passes translateTypeAnnotation as a
parameter.

So the chain of call seems like this :
buildSchema -> buildModuleSchema -> buildPropertySchema -> translateFunctionTypeAnnotation

siddarthkay · 2023-05-11T10:56:23Z

buildSchema is exported from parser-commons.js and imported in :

flow/parser.js
Here flowTranslateTypeAnnotation is imported from parsers/flow/modules/index.js which then contains
the actual definition of translateTypeAnnotation which is exported under the name of
flowTranslateTypeAnnotation
typescript/parser.js
Here typeScriptTranslateTypeAnnotation is imported from parsers/typescript/modules/index.js which
then contains the actual definition of translateTypeAnnotation which is exported under the name of
typeScriptTranslateTypeAnnotation

@cipolleschi : just sharing my debugging notes here for now, do share your perspective as well when you get a chance, I'd appreciate your insights on this one as well 🙏🏻

cipolleschi · 2023-05-11T14:20:35Z

uhm... your notes make sense... 🤔
I may have created a task that is too complex for now. Feel free to pause/drop it. I don't have an immediate solution right now, I have to invest some time and play with the codebase to understand what we can do. 🤔

siddarthkay · 2023-05-11T15:45:34Z

I can pause on this one for now and maybe pick another one up.
How about [Codegen 87] ?

siddarthkay · 2023-05-14T02:32:29Z

I figured out the issue I also had to make changes in packages/eslint-plugin-specs/react-native-modules.js and now tests should pass 🤞🏻

cipolleschi · 2023-05-15T12:47:04Z

That's cool! Thank you so much for doing this!

cipolleschi · 2023-05-15T12:47:11Z

/rebase

facebook-github-bot · 2023-05-15T13:55:01Z

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

facebook-github-bot · 2023-05-17T10:50:05Z

@cipolleschi merged this pull request in e09d585.

tarunrajput · 2023-05-17T13:25:58Z

Hi @cipolleschi, I think that pull requests for 105, 106, and 107 were merged after 104. As a result, we now have some duplicate code, resolveTypeAnnotation function for example.

cipolleschi · 2023-05-18T07:30:10Z

uhm... I see. I created the stack using the ordinal numbers, so the 104 was at the bottom of the stack, and then I applied all the other changes. :(

Probably this was the results of a badly handled git commit resolution.

This should fix the problem: #37477

Is there any other duplicated code?

tarunrajput · 2023-05-18T10:16:06Z

Is there any other duplicated code?
No, #37477 should remove all duplicated code

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 May 11, 2023

siddarthkay changed the title ~~[Codegen 104] Add getResolvedTypeAnnotation to Parsers~~ [Codegen 104] Add getResolvedTypeAnnotation to Parsers May 11, 2023

siddarthkay marked this pull request as draft May 11, 2023 07:46

siddarthkay force-pushed the codegen-104 branch from 3cdaf7f to 36bc161 Compare May 11, 2023 09:57

siddarthkay force-pushed the codegen-104 branch 3 times, most recently from 99c9c97 to 0bd3f47 Compare May 14, 2023 02:32

siddarthkay marked this pull request as ready for review May 14, 2023 02:39

siddarthkay added 2 commits May 15, 2023 12:49

Add getResolvedTypeAnnotation to Parsers

5a5dbcc

resolveTypeAnnotation is no longer exported

0b9544c

github-actions bot force-pushed the codegen-104 branch from 0bd3f47 to 0b9544c Compare May 15, 2023 12:49

facebook-github-bot closed this in e09d585 May 17, 2023

facebook-github-bot added the Merged This PR has been merged. label May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Codegen 104] Add `getResolvedTypeAnnotation` to Parsers #37373

[Codegen 104] Add `getResolvedTypeAnnotation` to Parsers #37373

siddarthkay commented May 11, 2023

analysis-bot commented May 11, 2023 •

edited

Loading

siddarthkay commented May 11, 2023 •

edited

Loading

siddarthkay commented May 11, 2023

siddarthkay commented May 11, 2023

cipolleschi commented May 11, 2023

siddarthkay commented May 11, 2023

siddarthkay commented May 14, 2023

cipolleschi commented May 15, 2023

cipolleschi commented May 15, 2023

facebook-github-bot commented May 15, 2023

facebook-github-bot commented May 17, 2023

tarunrajput commented May 17, 2023

cipolleschi commented May 18, 2023

tarunrajput commented May 18, 2023

[Codegen 104] Add getResolvedTypeAnnotation to Parsers #37373

[Codegen 104] Add getResolvedTypeAnnotation to Parsers #37373

Conversation

siddarthkay commented May 11, 2023

Summary:

Changelog:

Test Plan:

analysis-bot commented May 11, 2023 • edited Loading

siddarthkay commented May 11, 2023 • edited Loading

siddarthkay commented May 11, 2023

siddarthkay commented May 11, 2023

cipolleschi commented May 11, 2023

siddarthkay commented May 11, 2023

siddarthkay commented May 14, 2023

cipolleschi commented May 15, 2023

cipolleschi commented May 15, 2023

facebook-github-bot commented May 15, 2023

facebook-github-bot commented May 17, 2023

tarunrajput commented May 17, 2023

cipolleschi commented May 18, 2023

tarunrajput commented May 18, 2023

[Codegen 104] Add `getResolvedTypeAnnotation` to Parsers #37373

[Codegen 104] Add `getResolvedTypeAnnotation` to Parsers #37373

analysis-bot commented May 11, 2023 •

edited

Loading

siddarthkay commented May 11, 2023 •

edited

Loading