Skip to content

Fix crash setting zoomEnabled on MapView creation#3269

Merged
mfazekas merged 1 commit intornmapbox:mainfrom
davegurnell:main
Dec 14, 2023
Merged

Fix crash setting zoomEnabled on MapView creation#3269
mfazekas merged 1 commit intornmapbox:mainfrom
davegurnell:main

Conversation

@davegurnell
Copy link
Contributor

@davegurnell davegurnell commented Dec 13, 2023

Description

Fixes a bug where, in iOS, setting the zoomEnabled prop on MapView before the native component is created would cause a crash.

The code change is minimal and I've tested it in my own app. I'm having trouble getting a test case into the example app, though. Help with that would be appreciated. I'm on macOS Sonoma and I'm using Xcode 15.1.

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)

I'm afraid I'm unable to get the example app to build. I've done the following:

  • Change to the repo root directory
  • Run yarn generate and yarn build
  • Change to the example directory and run yarn install
  • Change to the example/ios directory and runpod install --repo-update and pod update MapboxMaps
  • Build and run from Xcode (I can't get yarn react-native run-ios working because of some Ruby problem)
  • Run yarn start

I get the following error message when I run the app:

CleanShot 2023-12-13 at 17 24 54

@davegurnell davegurnell temporarily deployed to CI with Mapbox Tokens December 13, 2023 17:07 — with GitHub Actions Inactive
@davegurnell davegurnell temporarily deployed to CI with Mapbox Tokens December 13, 2023 17:07 — with GitHub Actions Inactive
@davegurnell davegurnell temporarily deployed to CI with Mapbox Tokens December 13, 2023 17:07 — with GitHub Actions Inactive
@davegurnell davegurnell temporarily deployed to CI with Mapbox Tokens December 13, 2023 17:07 — with GitHub Actions Inactive
@davegurnell davegurnell temporarily deployed to CI with Mapbox Tokens December 13, 2023 17:07 — with GitHub Actions Inactive
@davegurnell davegurnell temporarily deployed to CI with Mapbox Tokens December 13, 2023 17:07 — with GitHub Actions Inactive
Copy link
Contributor

@mfazekas mfazekas left a comment

Choose a reason for hiding this comment

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

Thanks this looks good.

FWIW I'm considering deprecating zoomEnabled as we now have finer grain control with getstureSettings

@mfazekas mfazekas marked this pull request as ready for review December 14, 2023 10:15
@mfazekas mfazekas merged commit 03a8f2e into rnmapbox:main Dec 14, 2023
@mfazekas
Copy link
Contributor

Change to the repo root directory
Run yarn generate and yarn build
Change to the example directory and run yarn install
Change to the example/ios directory and runpod install --repo-update and pod update MapboxMaps
Build and run from Xcode (I can't get yarn react-native run-ios working because of some Ruby problem)
Run yarn start

That's interesting...

You do see the @rnmapbox/maps symlink in example/node_modules right?!

example % ls -la node_modules/@rnmapbox/maps 
lrwxr-xr-x@ 1 boga  staff  8 Dec  9 11:50 node_modules/@rnmapbox/maps -> ../../..

@mfazekas
Copy link
Contributor

@davegurnell what is your yarn version?! We might need yarn classic for the example to build

@davegurnell
Copy link
Contributor Author

davegurnell commented Dec 20, 2023 via email

@mfazekas
Copy link
Contributor

@davegurnell sorry I've overlooked and looked at the wrong node_modules directory. Yarn 3 should work as well.

You do see the @rnmapbox/maps symlink in example/node_modules right?!

example % ls -la node_modules/@rnmapbox/maps 
lrwxr-xr-x@ 1 boga  staff  8 Dec  9 11:50 node_modules/@rnmapbox/maps -> ../../..

@davegurnell
Copy link
Contributor Author

Oddly, that wasn't the problem but now I think it is.

I'm going around in circles with different versions of node, yarn, ruby, etc.

Don't worry about me - I'll work it all out and if I find anything useful I'll start a Discussion with some proposals for the README.

Thanks for your help and for quick publication of that fix.

Have a great Christmas!

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