-
Notifications
You must be signed in to change notification settings - Fork 534
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
Remove unsafe lifecycle methods (WIP) #849
Remove unsafe lifecycle methods (WIP) #849
Conversation
👋 Just wanted to say I support this work and thank you for doing it. I encountered this error just today and was considering abandoning use of this library in favor of direct Mapbox, and this PR made me stay. Looking forward to seeing this merged. Any assistance required? |
Mostly getting attention from the maintainers so that we can figure out a path towards merge. If you're interested in writing some code, I couldn't figure out why the |
I'm not too familiar with the codebase, but will try to take a look and see if I can figure anything out.. |
@mklopets too |
Hello, i should have some time to look into react-mapbox-gl in the coming months, i will look into this as a priority. Thanks |
Merged as part of #899, will add a few more and do a react-mapbox-gl release great work @tstirrat15 |
Hooray! |
Fixes #848
Motivation
The
UNSAFE
methods are going to be deprecated, and can't be used if you want to use fancy prerelease React features. This refactors away from those methods.Changes
UNSAFE_componentWillReceiveProps
tocomponentDidUpdate
UNSAFE_componentWillMount
tocomponentDidMount
Discussion
On the methods
willMount
anddidMount
should be directly interchangeable for the purposes of this library. All of the behavior in these methods is around setting up event handlers and interacting with the map, and given that there is minimal rendered state in the components, it shouldn't be a problem to render once, rundidMount
, and set up the handlers post-mount.willReceiveProps
doesn't have a direct analogue in the non-deprecated lifecycles. If you're setting state inwillReceiveProps
, it's typicallygetDerivedStateFromProps
that's used, but that doesn't happen often in this library.This library uses
willReceiveProps
most often to do side-effecty logic, which under the new paradigm is done indidUpdate
. The big difference betweendidUpdate
andwillReceiveProps
is thatdidUpdate
runs on mount, and there isn't a way to tell it not to run on mount.This is why I rewrote the tests as extensively as I did - they were implicitly depending on
willReceiveProps
not being called after mount. I'm not particularly happy with injectingmapInstance
, but I didn't have any other ways of doing it off the top of my head. I'm happy for feedback there.The skipped tests
One is for
scale-control
, and I can't figure out what's wrong with it. It complains aboutclientWidth
being undefined, despite that if youconsole.log
outmap._canvas
it shows an object with a key ofclientWidth
. Not really sure what's going on here.The other is for
center
being calculated whenfitBounds
is provided withoutcenter
, which is anonMount
behavior that only makes sense ifMap
is undefined.I suppose it might be possible to keep adding things to the old
mockMap
until test pass. If themapInstance
approach isn't desired I'll explore that.On my methods
I've never used typescript before. I'm sure I mangled at least a few things - please point them out.
TODO