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

Fix error in clusterMaxZoomLevel on new arch #3541

Merged
merged 3 commits into from
Jun 29, 2024

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Jun 21, 2024

Description

On new arch when the map re-renders it causes the following error when using the clusterMaxZoomLevel prop. This is because this prop cannot be updated in mapbox-ios. This isn't a problem in the old architecture since the prop handler is only called when the prop changes, in the new arch it needs to be handled manually by comparing the props values.

Mapbox [error] RNMBXShapeSource | clusterMaxZoomLevel  Cannot set property clusterMaxZoom for the source symbolLocationSource

After this the error no longer appears, unless actually changing the value of clusterMaxZoom.

It might be a good pattern to adopt generally for all components and props, but I thought it was out of scope of this since it would become a pretty big refactor.

Tested this change in an app that makes use of clustering.

Checklist

  • I've read CONTRIBUTING.md
  • I updated the doc/other generated code with running yarn generate in the root folder
  • I have tested the new feature on /example app.
    • In V11 mode/ios
    • In New Architecture mode/ios
    • In V11 mode/android
    • In New Architecture mode/android
  • I added/updated a sample - if a new feature was implemented (/example)

Screenshot OR Video

Component to reproduce the issue you're fixing

@mfazekas
Copy link
Contributor

@janicduplessis thanks that is great.

One approach I was experimenting with is:

RNMBX_OPTIONAL_PROP_NSString(puckBearing)
RNMBX_OPTIONAL_PROP_BOOL(puckBearingEnabled)
RNMBX_OPTIONAL_PROP_NSString(bearingImage)
RNMBX_OPTIONAL_PROP_NSString(shadowImage)
RNMBX_OPTIONAL_PROP_NSString(topImage)
RNMBX_OPTIONAL_PROP_ExpressionDouble(scale)
RNMBX_PROP_BOOL(visible)
RNMBX_OPTIONAL_PROP_NSDictionary(pulsing)

Those macros are defined here:
https://github.com/rnmapbox/maps/blob/main/ios/RNMBX/RNMBXFabricPropConvert.h

What do you think?

@janicduplessis
Copy link
Contributor Author

Looks good, I've seen this approach used too in some components in react-native core. I will update RNMBXShapeSourceComponentView to use those.

@janicduplessis janicduplessis temporarily deployed to CI with Mapbox Tokens June 29, 2024 03:39 — with GitHub Actions Inactive
@janicduplessis janicduplessis temporarily deployed to CI with Mapbox Tokens June 29, 2024 03:39 — with GitHub Actions Inactive
@janicduplessis janicduplessis temporarily deployed to CI with Mapbox Tokens June 29, 2024 03:39 — with GitHub Actions Inactive
@janicduplessis janicduplessis temporarily deployed to CI with Mapbox Tokens June 29, 2024 03:39 — with GitHub Actions Inactive
@janicduplessis janicduplessis temporarily deployed to CI with Mapbox Tokens June 29, 2024 03:39 — with GitHub Actions Inactive
@janicduplessis janicduplessis temporarily deployed to CI with Mapbox Tokens June 29, 2024 03:39 — with GitHub Actions Inactive
@janicduplessis janicduplessis temporarily deployed to CI with Mapbox Tokens June 29, 2024 03:39 — with GitHub Actions Inactive
@janicduplessis
Copy link
Contributor Author

@mfazekas Updated to use macros

@mfazekas mfazekas merged commit dbe96aa into rnmapbox:main Jun 29, 2024
11 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.

2 participants