feat: migrate Camera to new arch#3082
Conversation
mfazekas
left a comment
mfazekas
left a comment
There was a problem hiding this comment.
@WoLewicki nice, thanks much!
I'm don't think I fully understand the need and purpose for fabricexample. I'm concerned about code duplication. I'm sure some of the example will be improved in fabricexample some in example and it will get confusing.
Note that I've updated the example to rn 0.72.5 so it should work with fabric as well, I think I've briefly tried that. It's building in CI for sure. fabricexample still has simpler setup it uses symlink instead of metro/babel trickery.
| void setLogoPosition(T view, Dynamic value); | ||
| void setCompassEnabled(T view, boolean value); | ||
| void setCompassFadeWhenNorth(T view, boolean value); | ||
| void setCompassEnabled(T view, Dynamic value); |
There was a problem hiding this comment.
What's the reason for the codegen change? I was changed from Int, etc to all Dynamic in old-arch?
There was a problem hiding this comment.
I'm don't think I fully understand the need and purpose for fabricexample.
I just wanted to have both example and fabricexample with cached pods etc, but after we convert the whole library, we can just remove it.
What's the reason for the codegen change? I was changed from Int, etc to all Dynamic in old-arch?
I'll paste the reasoning I described with folks on codegen channel:
There was a problem hiding this comment.
I updated the PR with newest main and UnsafeMixed<T> code, which is cool btw :D
There was a problem hiding this comment.
Thanks, so just to clarify that I understand correctly.
logoEnabled?: boolean
is an optional property and by default we've showed the logo if it was shown by default by the native Mapbox SDK, which was true - I assume.
But in new architecture, if user doesn't set's logoEnabled logoEnabled gets default value as false, as codegen doesn't recognise the optional ? in the type declaration? So if you don't set logoEnabled in new architecture it'll be hidden.
Have you tested, WithDefault ?
logoEnabled?: WithDefault<boolean,undefined>;for old-arch it seems that it generated correct code:
- mViewManager.setLogoEnabled(view, value == null ? false : (boolean) value);
+ mViewManager.setLogoEnabled(view, value == null ? null : (Boolean) value);Haven't checked what it generated for new arch.
There was a problem hiding this comment.
The problem exists on iOS, on Android the values can be null indeed, but on iOS we have props as primitive cpp types with no Optional.
There was a problem hiding this comment.
@WoLewicki oh I see. I assume codegen could (and should) generate std::optional<boolean>. But yeah it doesn't.
Sure than it's good. The only improvement I can think of is this:
type OptionalProp<T> = UnsafeMixed<T>;
...
logoEnabled?: OptionalProp<boolean>;
...
}Hopefully codegen will have means of supporting optional properties, and then we can replace.
|
I resolved the issues. You still need to trigger the jobs for CI, right? |
|
@WoLewicki you have now access I think, so you can push it to a branch on this repo? then CI can run. |
|
Ok, I'll check |
|
@WoLewicki I'd like to update our example to rn 0.73 and will be requiring 0.73 for fabric going further. Is this an issue for you? |
|
If everything works OOTB then it is perfect for me.
Do you think there is much value in porting it now? |
2 participants
PR adding Camera implementation on new arch and adding all the files to fabricexample so it is easier to test new things there.