Skip to content

Conversation

@AntoineDoubovetzky
Copy link

Summary:

This PR contains task 117 from #34872:

[Codegen 117] Extract the code that throws if argumentProps are null in a throwIfArgumentPropsAreNull function in the error-utils.js file. Use it in the flow/components/events.js and in the typescript/components/event.js files

Changelog:

[Internal] [Changed] - Extract throwIfArgumentPropsAreNull function in error-utils

Test Plan:

I tested using Jest and Flow commands.

@facebook-github-bot 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 4, 2023
@analysis-bot
Copy link

analysis-bot commented May 4, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,732,589 -4
android hermes armeabi-v7a 8,043,921 -4
android hermes x86 9,221,935 -5
android hermes x86_64 9,074,911 -1
android jsc arm64-v8a 9,297,786 +0
android jsc armeabi-v7a 8,486,549 -3
android jsc x86 9,358,528 -4
android jsc x86_64 9,615,108 -5

Base commit: d8ced6f
Branch: main

Comment on lines 239 to 243
if (!argumentProps) {
throw new Error(`Unable to determine event arguments for "${name}"`);
throwIfArgumentPropsAreNull(argumentProps, name);
} else if (!bubblingType) {
throwIfBubblingTypeIsNull(bubblingType, name);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified to just:

throwIfArgumentPropsAreNull(argumentProps, name);
throwIfBubblingTypeIsNull(bubblingType, name);

Copy link
Author

@AntoineDoubovetzky AntoineDoubovetzky May 4, 2023

Choose a reason for hiding this comment

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

I tried, but there is a Flow error in the returned object because Flow doesn't infer that argumentProps and bubblingType cannot be null.
Do you know a way to fix this?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, the goal is to remove the if-else as Dmitry suggested. 🤔

one way could be to:

  1. Define a type alias like type ArgumentProps = $FlowFixMe
  2. Have throwIfArgumentPropsAreNull return ArgumentProps (not nullable)
  3. invoke it like argumentProps = throwIfArgumentPropsAreNull(argumentProps, name);

A bit ugly, but it should do the trick.

Otherwise, we can actually suppress the warning with a // eslint-disable-next-line <name of the broken rule> as we are actually sure that the props can't be null now.

I'm okay with both approaches!

Copy link
Author

Choose a reason for hiding this comment

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

I chose the first option. Even though I don't like having to make the code more complex for typing reasons, I think it's safer.

@dmytrorykun dmytrorykun requested a review from cipolleschi May 4, 2023 12:52
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to work on this changes. I left a comment to help solving the situation!

@cipolleschi
Copy link
Contributor

Thank you for working on this and to solve the issue. Do you have time to also rebase and fix the conflicts, please? 🙏

@AntoineDoubovetzky AntoineDoubovetzky force-pushed the refactor/extract-throwIfArgumentPropsAreNull-from-events branch from 5cd2cd7 to 5450045 Compare May 11, 2023 15:57
@AntoineDoubovetzky
Copy link
Author

Thank you for working on this and to solve the issue. Do you have time to also rebase and fix the conflicts, please? 🙏

Done!

@cipolleschi
Copy link
Contributor

/rebase

@github-actions github-actions bot force-pushed the refactor/extract-throwIfArgumentPropsAreNull-from-events branch from 5450045 to 0b9cf6e Compare May 15, 2023 12:26
@facebook-github-bot
Copy link
Contributor

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

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

@cipolleschi merged this pull request in f05252a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants