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: migrate RNMBXSources to new arch #3123

Merged
merged 9 commits into from
Oct 19, 2023
Merged

Conversation

WoLewicki
Copy link
Contributor

@WoLewicki WoLewicki commented Oct 19, 2023

PR adding New Architecture support to the library 🎉

We at Software Mansion have been working on improving support for the new architecture for quite a while now. If you need help with anything related to New Architecture, like:

or you just want to ask any questions, hit us up on [email protected]


Notes

PR migrating all RNMBXSources to new arch.

@WoLewicki
Copy link
Contributor Author

@mfazekas I have a few questions with this PR:

  1. I can see features native command in both ShapeSource and VectorSource components, but they are not implemented on the native side. Can I just remove them? I added a module for ShapeSource to better handle it:
    async features(filter: Array<string> = []) {
  2. I'd like to remove the whole NativeBridgeComponent (https://github.com/rnmapbox/maps/blob/cd4379c98670ea6e6a561e8ebbe76c7f6e0247a0/src/components/NativeBridgeComponent.tsx) class since it won't be used after we transfer commands to just native methods. Is it ok? Should I know something particular about it when deleting?
  3. Can I leave the basic implementation of getting the ShapeSource view in the module or should I do something similar to this PR: j-piasecki@c60584d ? It seems that the methods on ShapeSource are not called immediately after the components are rendered but I may be missing something.

Other than that, the PR seems to be ready. I'd remove the NativeBridgeComponent in the next PR when migrating PointAnnotation since it also uses this class.

@mfazekas
Copy link
Contributor

@mfazekas I have a few questions with this PR:

  1. I can see features native command in both ShapeSource and VectorSource components, but they are not implemented on the native side. Can I just remove them? I added a module for ShapeSource to better handle it:
    async features(filter: Array<string> = []) {

This was implemented in v8 but was not yet implemented in v10 this can be deleted. Now

  1. I'd like to remove the whole NativeBridgeComponent (https://github.com/rnmapbox/maps/blob/cd4379c98670ea6e6a561e8ebbe76c7f6e0247a0/src/components/NativeBridgeComponent.tsx) class since it won't be used after we transfer commands to just native methods. Is it ok? Should I know something particular about it when deleting?

Sure this can be deleted.
We still have some common code that might needs to be shared in some way:

https://github.com/rnmapbox/maps/blob/b08319a2a23440153c36d900c1490bc718645856/src/components/MapView.tsx#L921-L945C4

  1. Can I leave the basic implementation of getting the ShapeSource view in the module or should I do something similar to this PR: j-piasecki@c60584d ? It seems that the methods on ShapeSource are not called immediately after the components are rendered but I may be missing something.

I think we'll face the same issue if the user takes a ref and calls the commend as soon as the ref is available.
I think we can share a single ViewTagResolver across multiple view managers. But this is something I'm ok to be done later.

Other than that, the PR seems to be ready. I'd remove the NativeBridgeComponent in the next PR when migrating PointAnnotation since it also uses this class.

@WoLewicki
Copy link
Contributor Author

Ok thanks for answering the comments, I'll remove the features for now then and will do the rest of the cleanup concerning other components in next PRs 🚀

@mfazekas mfazekas merged commit 19bb63a into main Oct 19, 2023
9 checks passed
@mfazekas mfazekas deleted the @wolewicki/migrate-sources branch October 19, 2023 20:33
@WoLewicki
Copy link
Contributor Author

After checking deeper, I think I'll leave the NativeBridgeComponent, but only with the _runPendingNativeCommands since it seems reasonable to have it. Wdyt?

@mfazekas
Copy link
Contributor

@WoLewicki I think that's ok. (FWIW if we can me it into composition instead of inheritance and HOC that's even better, but it's not critical.)

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