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

perf: ensure we do not provide callback to native if no callback provided from app #3735

Merged

Conversation

freeboub
Copy link
Collaborator

@freeboub freeboub commented May 5, 2024

Summary

As discussed with @KrzysztofMoch a possible optimization is to avoid giving callback functions (starting with 'on*') to native if application didn't provide it

Motivation

performances improvements.
Additionnally, we must keep the useCallback as paramters of the function are differents between native & applicative, for exemple:

    const _onTimedMetadata = useCallback(
      (e: NativeSyntheticEvent<OnTimedMetadataData>) => {
        onTimedMetadata?.(e.nativeEvent);
      },
      [onTimedMetadata],
    );

send e.nativeEvent to application

Changes

Just add ternary to check to avoid giving the callback to native

Test plan

Basic tests done on android/ios with the sample

@KrzysztofMoch
Copy link
Collaborator

KrzysztofMoch commented May 5, 2024

I also think we should do "early returns" - because we don't need to load tracks data if user don't revive it

I will send you patch for iOS later
I will make separate PR for this

Copy link
Collaborator

@KrzysztofMoch KrzysztofMoch left a comment

Choose a reason for hiding this comment

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

We need to add "or" condition to places that "poster" logic depends on

like

onVideoLoad={
  onLoad || hasPoster
    ? (onVideoLoad as (e: NativeSyntheticEvent<object>) => void)
    : undefined
}

src/Video.tsx Outdated Show resolved Hide resolved
src/Video.tsx Outdated Show resolved Hide resolved
@freeboub
Copy link
Collaborator Author

freeboub commented May 6, 2024

Good catch agin, thank you !
I integrate some of the swift fix you proposed to me also ! (but not all)

Copy link
Collaborator

@KrzysztofMoch KrzysztofMoch left a comment

Choose a reason for hiding this comment

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

Nice let's merge this - I will make PR for "early returns" tomorrow

@KrzysztofMoch KrzysztofMoch merged commit c59d00a into TheWidlarzGroup:master May 7, 2024
8 checks passed
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