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

feat: make Camera pure #471

Merged
merged 29 commits into from
Nov 11, 2024
Merged

Conversation

KiwiKilian
Copy link
Collaborator

@KiwiKilian KiwiKilian commented Oct 21, 2024

From Slack my initial motivation:

I’m thinking the migration of UNSAFE_componentWillReceiveProps in Camera could be simpler. I think the native part get’s currently updated twice/multiple times:
By setting the props= to the RCTMLNCamera
And by calling setNativeProps for most props in an useEffect
Was there a reasoning behind doing both?
I will be drafting a PR which only uses setNativeProps for the imperative methods. I’ve mostly compared our current version with the RNMapbox current Camera . I know we are diverging but it but their technique for migration to hooks here seems just as fine for MapLibre. Note: We can still look at RNMapox for such topics – it’s MIT as it’s still only by the community. Also they moved forward to the new architecture and Swift and Kotlin. Lots of work to do for RN MapLibre 😅.
It will keep allowUpdates broken. It currently only blocks the imperative calls anyway. And I don’t see a way for this to work on the React (TS/JS) side. We would have to keep the previous props in state (refs aren’t allow to read/write during render). That would mean extra re-renders. I’m thinking it would be better to implement this on the native side, if anybody uses this. Does someone have a usecase/example for this?

@KiwiKilian KiwiKilian marked this pull request as ready for review November 7, 2024 06:16
@KiwiKilian
Copy link
Collaborator Author

KiwiKilian commented Nov 7, 2024

I think this one is now ready to merge. I tested quite a bit within our app and the camera behavior seems stable to me now.

What I did:

  • Make the Camera component pure
    • No more side effects, just plainly passing the props to native and exposing the imperative methods
    • Imperative calls were janked previously and changing props would cause multiple re-renders
  • Changed the default animationMode for controlled Camera to CameraMode.None
    • If one uses the controlled Camera (by setting coordinates etc. as props or using setCamera), it should also be mandatory to choose the animation
  • Dropped the allowUpdates prop
    • It's opposing to pure components, because the order of changing props would matter
    • Should be handled in user space, if necessary (e.g. not updating the controlled Camera props, when condition xy)
  • Removed the unimplemented triggerKey prop
    • Should also be possible with imperative call setCamera
  • Externalized some static functions like makeNativeBounds and makeNativeStop
  • Remove animationDuration and animationMode as props to the native iOS camera
    • These were unused and are passed via the stop

Copy link
Contributor

@caspg caspg left a comment

Choose a reason for hiding this comment

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

That's really great change. Looking good.

@tyrauber tyrauber merged commit 23ecf88 into maplibre:main Nov 11, 2024
4 checks passed
@KiwiKilian KiwiKilian deleted the feat/camera-refactor branch November 13, 2024 06:10
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.

3 participants