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

[react-native-vision-camera] Add definition #4533

Merged
merged 3 commits into from Oct 12, 2023
Merged

[react-native-vision-camera] Add definition #4533

merged 3 commits into from Oct 12, 2023

Conversation

joshuanapoli
Copy link
Contributor

@@ -0,0 +1,1358 @@
declare module "react-native-vision-camera" {
// import type { ViewProps } from "react-native";
Copy link
Member

Choose a reason for hiding this comment

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

React native is flow-typed but not available in the test environment.. We should probably aim to add this into the harness so that react-native packages can resolve properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah; importing from react-native would be nice. I think that the react-native dependency would still be impossible in flow-typed v4, since react-native types aren't supplied through this project.

Copy link
Member

Choose a reason for hiding this comment

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

I'll raise an issue to track, I think I can do something with the CLI later so that we can test with react-native as a dependency

Copy link
Member

Choose a reason for hiding this comment

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

@joshuanapoli I need some help. How does react-native ship flow definitions? As I was creating the change i realised that there are no .js.flow files as far as I can tell in the react-native package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Brianzchen The source files in the shipped package are all flow-typed. As far as I understand, the package.json refers to these flow-typed files. As configured by package.json, the flow-typed files will be loaded at "run time" by import/requre. There are no pure JavaScript files in the package. Also, configured by package.json, Typescript .d.ts files are loaded from the types directory.

Each of the main sources have a flow comment:

/**
 [...]
 * @format
 * @flow strict-local
 */

Evidently, the metro bundler strips the flow types.

I hope that this information is correct. It seems surprising to me.

| CaptureError
| SystemError
| UnknownError;
declare class CameraError<TCode: CameraErrorCode> mixins Error {
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to not use mixins here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks; the "mixins" are replaced with "extends" in c277db7

*/
frameProcessorFps?: number | "auto",
...
} & ViewProps;
Copy link
Member

Choose a reason for hiding this comment

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

generally prefer this as it's usually what people intend when they port from TS definitions

{
 ...ViewProps,
 // other stuff
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed in cff3d5b

@Brianzchen Brianzchen merged commit 590dbb1 into flow-typed:master Oct 12, 2023
4 checks passed
@Brianzchen
Copy link
Member

#4535
Made a start on something that looks quite promising! Will continue on this later as I'll be busy this weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants