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

Typescript check for FFprobeKit.getMediaInformationAsync #197

Closed
iclassPeter opened this issue Oct 21, 2021 · 9 comments
Closed

Typescript check for FFprobeKit.getMediaInformationAsync #197

iclassPeter opened this issue Oct 21, 2021 · 9 comments
Assignees
Labels
enhancement New feature or request fixed-in-v4.5.1 library Affects the library react-native Affect react-native platform v4.5 Affects v4.5 release

Comments

@iclassPeter
Copy link

iclassPeter commented Oct 21, 2021

Description
Not functional issue, just typescript check. Typescript not match in argument of getMediaInformationAsync under FFprobeKit.

Expected behavior
The second argument of getMediaInformationAsync is in type ExecuteCallback = (session: Session) => void; but the session return should be MediaInformationSession. probably having same issue on getMediaInformationFromCommandAsync and getMediaInformationFromCommandArgumentsAsync under FFprobeKit.

Current behavior
Argument of type '(session: MediaInformationSession) => Promise' is not assignable to parameter of type 'ExecuteCallback'.
Types of parameters 'session' and 'session' are incompatible.
Type 'Session' is missing the following properties from type 'MediaInformationSession': getMediaInformation, setMediaInformation

To Reproduce

FFprobeKit.getMediaInformationAsync(
                        path, 
                        async (session:MediaInformationSession) => { 
                            const information:MediaInformation = await session.getMediaInformation();
                            const duration = information.getDuration();
                        }
)

Screenshots
Screenshot 2021-10-21 at 3 39 04 PM

Environment

  • Platform: React Native
  • Version: ^4.5.0 , full-gpl-lts
  • Source branch: react native
@tanersener
Copy link
Collaborator

tanersener commented Oct 21, 2021

ExecuteCallback is used by other API methods as well. And, they don't return MediaInformationSession. For example, some methods return FFmpegSession. If I make this change they will complain. That's why it is defined as generic Session.

I can create separate Callbacks for each method, e.g. MediaInformationSessionCallback, FFprobeSessionCallback, FFmpegSessionCallback. Will that work for you, or will it make it too complicated?

@tanersener tanersener added library Affects the library react-native Affect react-native platform v4.5 Affects v4.5 release labels Oct 21, 2021
@iclassPeter
Copy link
Author

iclassPeter commented Oct 22, 2021

Separate Callbacks types would work. It might actually ease new user calling the api, as correct type helps autofill functions and confirm they are useable. Btw thank you for making this library.


P.S. I dont know what could be gone too complicated if changing type as I am new to typescript and sessions actually.

What I gone through without exact correct type:
I didnot know session.getMediaInformation return type MediaInformation from reading doc so I was confused on the [object] returned. I could only see [object] in console.log so I have looked into the declaration to find out what functions i could use.

Screenshot 2021-10-22 at 10 06 58 AM

@tanersener
Copy link
Collaborator

tanersener commented Oct 22, 2021

We have identical APIs across all platforms. These callback types are the same in all of them. Your problem is also a common one. Other platforms need to cast the session instance to the actual session type in order to call some of the methods, which is not very nice.

I'll mark this as an enhancement request and work on this. Thanks for reporting.

@tanersener tanersener added the enhancement New feature or request label Oct 22, 2021
@tanersener tanersener added this to Needs triage in Next GA Release via automation Oct 22, 2021
@iclassPeter
Copy link
Author

One could use Type Assertions if feeling needed to remove the red underline, but it's not changing the way of finding out the type of session.

FFprobeKit.getMediaInformationAsync(
    path, 
    async (session) => {
        const information = await (session as MediaInformationSession).getMediaInformation();
        const duration = information.getDuration();
    }
);

@tanersener
Copy link
Collaborator

Yeah, you can use that if you are developing using TypeScript.

Next GA Release automation moved this from Needs triage to Closed Nov 1, 2021
@iclassPeter iclassPeter reopened this Nov 1, 2021
Next GA Release automation moved this from Closed to Needs triage Nov 1, 2021
@tanersener tanersener moved this from Needs triage to High priority in Next GA Release Nov 5, 2021
@tanersener tanersener moved this from In Progress to Planned in Next GA Release Dec 3, 2021
@tanersener tanersener moved this from Planned to In Progress in Next GA Release Dec 23, 2021
@tanersener tanersener moved this from In Progress to Closed in Next GA Release Dec 25, 2021
@tanersener tanersener self-assigned this Jan 3, 2022
@tanersener
Copy link
Collaborator

Fixed in v4.5.1.

@aklemen
Copy link

aklemen commented Jan 5, 2022

Hi, could you please provide an example for calling FFprobeKit.getMediaInformationAsync() with appropriate types (in React Native)?

I noticed that in the new version v4.5.1 the executeCallback?: ExecuteCallback (which receives generic Session as an argument) has been changed to completeCallback?: FFprobeSessionCompleteCallback (which receives FFProbeSession as an argument). Should the type for FFprobeKit.getMediaInformationAsync()'s completeCallback be MediaInformationSessionCallback (with MediaInformationSession as in @iclassPeter's example)?

@tanersener
Copy link
Collaborator

tanersener commented Jan 5, 2022

@aklemen ffmpeg-kit-test project includes examples for most of the API methods. We don't have a typescript application yet. But typescript definitions are there, they will help you about the types.

@aklemen
Copy link

aklemen commented Jan 5, 2022

@tanersener I tried one of the examples before, but it didn't work (got error Typescript: undefined is not a function).
I cleared the npm cache and did a clean install of the RN app and now it works.

I'll leave this simple example (to get duration in seconds) here, if anyone needs it in the future.

FFprobeKit.getMediaInformation(filepath)
  .then(async (session: MediaInformationSession) => {
    const information: MediaInformation = await session.getMediaInformation();
    const durationInSeconds: number = information.getDuration();
  })
  .catch(error => console.error(error));

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed-in-v4.5.1 library Affects the library react-native Affect react-native platform v4.5 Affects v4.5 release
Projects
No open projects
Next GA Release
  
Closed
Development

No branches or pull requests

3 participants