Skip to content
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.

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

Move two more libdefs into .js.flow files #5314

Merged
merged 11 commits into from
Apr 1, 2022

Conversation

chrisbobbe
Copy link
Contributor

Following #5311, which did this for react-intl. Now we do:

  • react-native-safe-area-context
  • @react-native-community/netinfo

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @chrisbobbe! All looks good modulo two small comments below -- then please merge at will.

declare export function useSafeAreaFrame(): Rect;
declare export function withSafeAreaInsets<P: { ... }>(
WrappedComponent: React$ComponentType<$Exact<P>>,
): React$AbstractComponent<{| ...$Exact<P>, +ref?: React$Ref<$Exact<P>> |}, mixed>;
Copy link
Member

Choose a reason for hiding this comment

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

For where the (translated) TypeScript got such things as
ForwardRefExoticComponent, see
  https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts

nit: link to a specific commit ID, so that the link continues to work in the future and point to the same code you had in mind when you wrote this commit message.

(No need to go and forensically track down what version the author of this bit of TypeScript might have had in mind -- whatever permalink you get when you hit y on that GitHub web page is fine, if it conveys the information you're thinking of when you write the commit message.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, TIL about y! 🙂 Thanks!

declare export function useSafeAreaFrame(): Rect;
declare export function withSafeAreaInsets<P: { ... }>(
WrappedComponent: React$ComponentType<$Exact<P>>,
): React$AbstractComponent<{| ...$Exact<P>, +ref?: React$Ref<$Exact<P>> |}, mixed>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
): React$AbstractComponent<{| ...$Exact<P>, +ref?: React$Ref<$Exact<P>> |}, mixed>;
): React$ComponentType<{| ...$Exact<P>, +ref?: React$Ref<$Exact<P>> |}>;

I had a recollection that AbstractComponent could often be simplified, and went and looked it up here:
https://github.com/zulip/zulip-mobile/blob/main/docs/background/react.md

Indeed, the key line is (from react.js):

declare type React$ComponentType<-Config> = React$AbstractComponent<Config, mixed>;

That's also nice because it makes the relationship between argument and return type more apparent.

NetInfoConnectedDetails is inexact; so, make the object type that it
contributes to (at NetInfoConnectedState.details) inexact.

Also, the type param D is often inexact; that's another reason to
make the outer object inexact.

As a consequence, Flow v0.149 would rightly complain about possibly
trying to `logging.warn` non-JSONable contents of the object. So,
comment about that.
To see that only whitespace is changed, view the diff with

  git diff -w
@chrisbobbe chrisbobbe force-pushed the pr-two-more-libdefs branch from 0048b30 to 044733e Compare April 1, 2022 00:23
@chrisbobbe chrisbobbe merged commit 044733e into zulip:main Apr 1, 2022
@chrisbobbe
Copy link
Contributor Author

Thanks for the review! Merged, with those fixes.

@chrisbobbe chrisbobbe deleted the pr-two-more-libdefs branch April 1, 2022 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants