- Notifications
You must be signed in to change notification settings - Fork1.3k
[react-native-vision-camera] Add definition#4533
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[react-native-vision-camera] Add definition#4533
Uh oh!
There was an error while loading.Please reload this page.
Conversation
joshuanapoli commentedOct 12, 2023
- Links to documentation:https://react-native-vision-camera.com/
- Link to GitHub or NPM:https://github.com/mrousavy/react-native-vision-camera
- Type of contribution: new definition
| @@ -0,0 +1,1358 @@ | |||
| declare module "react-native-vision-camera" { | |||
| // import type { ViewProps } from "react-native"; | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 doesreact-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 thereact-native package
joshuanapoliOct 16, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
joshuanapoliOct 12, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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" inc277db7
| */ | ||
| frameProcessorFps?: number | "auto", | ||
| ... | ||
| } & ViewProps; |
There was a problem hiding this comment.
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}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks. Fixed incff3d5b
Brianzchen commentedOct 12, 2023
#4535 |