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

Prepare for Fabric / TurboModules #650

Closed
wants to merge 2 commits into from
Closed

Conversation

mrousavy
Copy link
Owner

… Module (1/3)

What

Prepares VisionCamera for Fabric and TurboModules. Some notable changes:

Fabric

TurboModules

Changes

whole lotta changes.

Tested on

  • iPhone 11 Pro

@mrousavy mrousavy added 🐛 bug Something isn't working ✨ enhancement 🍏 ios Issue affects the iOS platform 🤖 android Issue affects the Android platform 💄 types Issue affects TypeScript typings 🛠 dependencies Pull requests that update a dependency file ✨ feature Proposes a new feature or enhancement 💨 performance This issue or pull requests addresses performance issues ☕️ js Affects the JS/TS codebase labels Dec 13, 2021
try {
return await CameraModule.takePhoto(this.handle, options ?? {});
return await this.handle.takePhoto(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

[tsc] <2339> reported by reviewdog 🐶
Property 'takePhoto' does not exist on type 'RefType'.

if (Platform.OS !== 'android')
throw new CameraCaptureError('capture/capture-type-not-supported', `'takeSnapshot()' is not available on ${Platform.OS}!`);

try {
return await CameraModule.takeSnapshot(this.handle, options ?? {});
return await this.handle.takeSnapshot(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

[tsc] <2339> reported by reviewdog 🐶
Property 'takeSnapshot' does not exist on type 'RefType'.

try {
CameraModule.startRecording(this.handle, passThroughOptions, onRecordCallback);
await this.handle.startRecording(passThroughOptions, onRecordCallback);
Copy link
Contributor

Choose a reason for hiding this comment

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

[tsc] <2339> reported by reviewdog 🐶
Property 'startRecording' does not exist on type 'RefType'.

@@ -205,7 +204,7 @@ export class Camera extends React.PureComponent<CameraProps> {
*/
public async stopRecording(): Promise<void> {
try {
return await CameraModule.stopRecording(this.handle);
return await this.handle.stopRecording();
Copy link
Contributor

Choose a reason for hiding this comment

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

[tsc] <2339> reported by reviewdog 🐶
Property 'stopRecording' does not exist on type 'RefType'.

@@ -231,7 +230,7 @@ export class Camera extends React.PureComponent<CameraProps> {
*/
public async focus(point: Point): Promise<void> {
try {
return await CameraModule.focus(this.handle, point);
return await this.handle.focusPoint(point);
Copy link
Contributor

Choose a reason for hiding this comment

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

[tsc] <2339> reported by reviewdog 🐶
Property 'focusPoint' does not exist on type 'RefType'.

@nbpro
Copy link

nbpro commented Dec 18, 2021

Hi @marc Rousavy any Idea by when this PR will be merged ?

@astanb
Copy link
Contributor

astanb commented Jan 7, 2022

Got the champagne on ice for this being merged, will finally be able to ditch all other camera libraries and use only this

🥳

@mrousavy
Copy link
Owner Author

mrousavy commented Jan 7, 2022

Why only after this PR is merged?

@astanb
Copy link
Contributor

astanb commented Jan 7, 2022

Unfortunately I've got an issue with needing to use a camera in a modal, see #500. Making do with expo-camera in that case.

@mrousavy
Copy link
Owner Author

mrousavy commented Jan 7, 2022

I personally never use <Modal>, it's better to separate the component into a separate Screen and make that present in Modal style anyways (readability as well as performance). That would solve your issue.

@astanb
Copy link
Contributor

astanb commented Jan 7, 2022

Yeah that's a possibility but then I have to deal with passing data back to the parent screen either by using global state, callbacks or via navigation params, none of which are particularly desirable for my use case.

@steffisturm
Copy link

Hi @mrousavy, I was wondering if you've had the time to work on this PR. Do you have any update on this? I'm currently working a project that has the same issue #570 on Android as well.

@mrousavy
Copy link
Owner Author

Hi @steffisturm, I'm not sure if you understand what the goal of this PR is. Is your app already running on Fabric and TurboModules?

@FDiskas
Copy link

FDiskas commented Jan 18, 2023

Please don't stop it here :) keep going.

@mrousavy
Copy link
Owner Author

Please don't stop it here :) keep going.

I don't have any budget for this right now. Don't have the free time to work on this either 🤷‍♂️

@mrousavy mrousavy closed this Sep 4, 2023
@mrousavy mrousavy deleted the prepare-for-fabric branch September 4, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment