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.

Sign up for GitHub

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

Jump to bottom

feat: make Camera pure #471

Merged
merged 29 commits into from
Nov 11, 2024
Merged

feat: make Camera pure #471

merged 29 commits into from
Nov 11, 2024

Conversation

KiwiKilian
Copy link
Collaborator

@KiwiKilian KiwiKilian commented Oct 21, 2024 •
edited
Loading

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
chore: configure jest to use with ts
0dd1dda
@KiwiKilian
feat: make Camera pure
6c99a95
@KiwiKilian
chore: update Fit example for map style
c10bf5f
@KiwiKilian
test: adapt Camera tests for pure rendering
9f0566f
@KiwiKilian
docs: generate
ffa55e7
@KiwiKilian
docs: improve Camera docs
caca6ac
@KiwiKilian
Merge branch 'main' into feat/jest-typescript
c18c4d8 
# Conflicts:
#	jest.config.js
#	setup-jest.js
@KiwiKilian
Merge branch 'feat/jest-typescript' into feat/camera-refactor
12d62df 
# Conflicts:
#	__tests__/components/Camera.test.js
#	packages/examples/src/examples/Camera/Fit.js
@KiwiKilian
fix: _trackingModeToString return type warning
f334ec7
@KiwiKilian
refactor: remove unused animationDuration and animationMode
2e03cd1
@KiwiKilian
fix: align CameraMode
d9f05e0
@KiwiKilian
test: update test for aligned animation modes
2f614a9
@KiwiKilian
feat: remove allowUpdates from Camera
1cec06c
@KiwiKilian
Merge branch 'main' into feat/camera-refactor
3f67c53 
# Conflicts:
#	tsconfig.json
@KiwiKilian
Merge branch 'main' into feat/camera-refactor
ce71bbe 
# Conflicts:
#	javascript/components/Camera.tsx
@KiwiKilian
Merge branch 'main' into feat/camera-refactor
3bb0f36
@KiwiKilian
chore: improve FollowUserLocationRenderMode example with raster tiles
679b327
@KiwiKilian
fix: separate makeNativeStop to reduce rerendering
d7e169d
@KiwiKilian
test: remove omits CameraStop with followUserLocation
2db437b
@KiwiKilian
docs: update imperative camera methods examples to ref
50aa4cc
@KiwiKilian
fix: remove useCallbacks for refs
161de55
@KiwiKilian
docs: generate
a0537f8
@KiwiKilian
docs: fix Camera docs
e2b11ff
@KiwiKilian
chore: add changelog entry
3f47f35
@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 •
edited
Loading

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

caspg
caspg approved these changes Nov 11, 2024
View reviewed changes
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
@tyrauber tyrauber mentioned this pull request Nov 12, 2024
Closed
@KiwiKilian KiwiKilian deleted the feat/camera-refactor branch November 13, 2024 06:10
github-actions bot pushed a commit that referenced this pull request Dec 2, 2024
@semantic-release-bot
chore(release): 10.0.0-beta.1 [skip ci]
4d982af 
# [10.0.0-beta.1](v9.1.0...v10.0.0-beta.1) (2024-12-02)

### Bug Fixes

* add @babel/plugin-transform-private-methods for jest ([3a2188f](3a2188f))
* add generic expo config plugin to remove duplicate signature ([#453](#453)) ([2671381](2671381))
* allow MapView and Images to have no children ([#521](#521)) ([1e35bf6](1e35bf6))
* allow resetting contentInset with 0 ([#468](#468)) ([1fe42c6](1fe42c6))
* android example crashing on launch ([#372](#372)) ([aeef5c3](aeef5c3))
* cleanup yarn setup ([#463](#463)) ([d9a4d30](d9a4d30))
* corepack enable  on publish workflow ([2d13f33](2d13f33))
* correct types in MapView ([#268](#268)) ([0ea35c4](0ea35c4))
* disable code signing for release builds ([b3cf088](b3cf088))
* disable library code signing ([22030dd](22030dd))
* empty pbxproj and dwarf-with-dsym plugin config for EAS ([#458](#458)) ([0d54b46](0d54b46))
* expo-app should load library from workspace:. ([016b44a](016b44a))
* group dependabot commits by core, dev and example ([#165](#165)) ([b697978](b697978))
* keep [@ts-ignore](https://github.com/ts-ignore) for headingIcon in library [#476](#476) ([#477](#477)) ([ef62454](ef62454))
* make MarkerView props with defaults optional ([#460](#460)) ([185cf3e](185cf3e))
* plugin for debug simulator ([#164](#164)) ([06b23d4](06b23d4))
* remove AbortController test mock ([#403](#403)) ([698b558](698b558))
* round compass margins and attribution position to nearest integers [android] ([#294](#294)) ([c89c842](c89c842))
* setMaxAnimationFps on null ([#440](#440)) ([2884256](2884256))
* style expressions ([#466](#466)) ([2202908](2202908))
* updated Mapbox callstack check for iOS custom headers to check for MapLibre instead ([#461](#461)) ([a6d6216](a6d6216))
* use UIManager exported from react-native ([#511](#511)) ([a4030b5](a4030b5))
* yarn implementation ([#419](#419)) ([39233b1](39233b1))

### Continuous Integration

* add semantic release ([#526](#526)) ([069b6c5](069b6c5))

### Features

* align react and react-native versions for development ([b92abfe](b92abfe))
* configure packages/examples ([c4510c3](c4510c3))
* extract android UserLocation FPS ([#428](#428)) ([8c0abaa](8c0abaa))
* make Camera pure ([#471](#471)) ([23ecf88](23ecf88))
* MapLibre Android SDK 11.5.0 ([#455](#455)) ([042b759](042b759))
* monorepo configuration ([343e7ac](343e7ac))
* mv example packages/react-native-app ([5c9d3d0](5c9d3d0))
* mv examples, styles, assets, utils and scenes to packages/examples ([13600fe](13600fe))
* packages/expo-app ([c01abd5](c01abd5))
* setup build step ([#504](#504)) ([a017d64](a017d64))
* shared dependencies through packages/examples ([01a9586](01a9586))
* support new arch through interop layer ([#483](#483)) ([951e9cf](951e9cf))
* update maplibre native version ([#61](#61)) ([25c418a](25c418a))
* upgrade [@Turf](https://github.com/turf) to v7 and remove geo utils ([#478](#478)) ([a45fc55](a45fc55))

### BREAKING CHANGES

* Upgrade native packages and migrate components

* ci: move native builds to review

* ci: run release immediate for debugging

* ci: use android working directory for build

* docs: remove RELEASE.md

* chore: remove manual changelog task

* ci: enable release on beta branch

* ci: keep default tagFormat

* ci: setup npm tag fixes

* ci: run review on mr to beta

* ci: run fix tags on beta

* ci: fix name

* ci: clarify workflow_call

* ci: disable debugging

* ci: run fix tags in pr

* ci: setup fix tags to run on beta

* docs: prepare changelog for semantic-release

* ci: remove fix npm tags workflow
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 2, 2024

🎉 This PR is included in version 10.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caspg caspg caspg approved these changes

Assignees
No one assigned
Labels
released on @beta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KiwiKilian @caspg @tyrauber