search

mapbox / mapbox-gl-draw

Draw tools for mapbox-gl-js
https://www.mapbox.com/mapbox-gl-js/example/mapbox-gl-draw/
ISC License
952 stars 593 forks source link

Eliminate dependence on Map#fire – breaking change in mapbox-gl-js #766

Open mollymerp opened 6 years ago

mollymerp commented 6 years ago

mapbox-gl-js recently changed the function signature for fire, so our most recent beta release (https://github.com/mapbox/mapbox-gl-js/pull/6549) breaks compatibility with this library. see change log here https://github.com/mapbox/mapbox-gl-js/releases/tag/v0.45.0-beta.1

mapbox-gl-draw shouldn't be relying on Map#fire because it is not a public method, but if that is impossible, another option would be to call fire with an object that has a type attribute {type: 'error', message: 'somethingbad', //... rest of the payload properties }. We may also consider making the Event class public so third party libraries like this can use that constructor.

samanpwbb commented 6 years ago

Fixed by https://github.com/mapbox/mapbox-gl-draw/commit/7eba80cae2b2b816b06119c09678f055881ce609

tristen commented 6 years ago

Mapbox GL Draw still draws heavy dependence on map.fire so I don't really see this as resolved.

jfirebaugh commented 6 years ago

Yeah, to be clear, map.fire is private as of https://github.com/mapbox/mapbox-gl-js/commit/2b645718743c4a5f229be9ab90df2a77328fbcce. It should not be called externally. A resolution to this issue would be for gl-draw to implement its own eventing mechanism and fire events on the MapboxDraw object.

I'd rather find a way for gl-js to restore backward compatibility than to treat 7eba80cae2b2b816b06119c09678f055881ce609 as a permanent solution.

samanpwbb commented 6 years ago

😅 definitely a premature close. We needed to get this fix in quickly for Studio tomorrow. The correct next action is to refactor gl-draw.

mcwhittemore commented 6 years ago

To be clear here, private or not, I screwed up here and exposed a dependencies functionality as core to Draw. This is made worse since custom modes are required to use this.map.fire to trigger draw events which means that to fix this we need to make a breaking change to Draw and rework how modes report events back.

All of that said, we should make this change, I just don't have time to really work on it right now. Even in mapbox-gl-js made map.fire handle both signatures, we shouldn't be exposing this like we are.

jfirebaugh commented 6 years ago

It's not just gl-draw that's affected by this -- see https://github.com/mapbox/mapbox-gl-js/issues/6522. So from a purely pragmatic standpoint, I think we need to revisit this in gl-js 0.45. I'm going to add a compatibility shim to fire such that it continues to accept a string and properties object, and we can work out a longer-term migration plan separately.