RCTMapboxGL should expose the underlying MGLMapView

[1ec5]

All these methods and the corresponding implementations could go away if RCTMapboxGL’s _map ivar is replaced with a public MGLMapView-typed mapView property (which could be accessed as a _mapView ivar inside RCTMapboxGL). Then, any time you need to do something to the MGLMapView inside RCTMapboxGLManager, you can use view.mapView.someProperty instead of having to implement yet another someProperty on RCTMapboxGL.

/cc @bsudekum

[dapetcu21]

I disagree. It helps with isolation. Especially since in #365 the MGLMapView might not be created before the style is set or before the view received its width and height. So, RCTMapboxGL is an abstraction that should hold even wether the underlying MGLMapView exists yet or not.

[1ec5]

If the intention is to essentially queue up property changes until MGLMapView exists, the current design makes some sense. But why not create MGLMapView when RCTMapboxGL is initialized, since there’s a one-to-one correspondence? An MGLMapView doesn’t have to be added to the view hierarchy immediately after being initialized.

Regardless, methods such as -setDebugActive: should be simplified. The current implementation is effectively identical to:

- (void)setDebugActive:(BOOL)debugActive
{
    _debugActive = debugActive;
    if (_debugActive != debugActive) _map.debugActive = debugActive;
}

There’s no need to check whether _map exists, because setting properties or calling methods on nil is a no-op in Objective-C. You won’t get a null-pointer exception as you would in Java or JavaScript.

[dapetcu21]

You're totally right. I thought this delayed load was done for a reason in the original implementation. This would simplify the code greatly.

Though, are you sure it's not cheaper to initialise it only after we know the frame? What happens with the underlying OpenGL texture if we call -init instead of -initWithFrame: and then relayout? Does it get initialised to a preset size, then gets resized when the view relayouts? If so, that's an expensive operation.

Also, I knew that sending messages to nil in Obj-C is a no-op. I just made a habit of checking anyway, for code clarity. If you don't like them, I'll remove them.

[1ec5]

There is a known issue with initializing the map view without a frame: mapbox/mapbox-gl-native#1572. I’ve been unable to reproduce it but apparently others keep running into it. The workaround is indeed to set an initial size like 10×10 and resize the view once you find out its eventual size. This is no different than what a native iOS application does when an Auto Layout–enabled view controller is set up, and it shouldn’t be any more expensive than loading the style in the first place.

The only thing I’d watch out for is that RNMBGL, at least in its original implementation, had a habit of blowing away the map view and reconstructing it any time a property changed. That behavior could be expensive and wasteful, and you’d want to avoid it if it’s still the case.

[1ec5]

The no-op thing isn’t a huge deal. I’m sure you aren’t alone in preferring explicit checks. Relying on the no-op behavior is very common in Objective-C mainly because the language is so verbose, but there’s no technical advantage. The one case I’d watch out for is if you need to chain property accesses. For instance, foo.bar.baz.boo = 1; can be a lot cheaper than if (foo && foo.bar && foo.bar.baz) foo.bar.baz.boo = 1; if those properties have custom getter implementations.

(I don’t meant to insult your intelligence; just want to write down some of the things @bsudekum and I have discussed in the past regarding this codebase.)

[dapetcu21]

Ok. Then I guess the course of action would be to initialise _map from the start and get rid of all the duplicated ivars. Maybe even make RCTMapboxGL a subclass of MGLMapView?

That destroy-and-reconstruct behaviour has since been fixed in my PR. It was one of the main reasons I made the PR in the first place. It now changes properties on _map as props come in.

[1ec5]

Wonderful! Yes, subclassing MGLMapView would simplify things even further.