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

Load layers after map style is loaded #86

Closed
kahama94 opened this issue Jun 10, 2019 · 6 comments · Fixed by #133
Closed

Load layers after map style is loaded #86

kahama94 opened this issue Jun 10, 2019 · 6 comments · Fixed by #133
Assignees
Labels
bug Something isn't working Gisida has pr help wanted Extra attention is needed Reveal

Comments

@kahama94
Copy link
Contributor

kahama94 commented Jun 10, 2019

Even after implementing myMap.on('style.load') to ensure map style is fully loaded the map may not be fully ready to load layers i.e styles haven't fully loaded. This has been pointed out as shown below mapbox/mapbox-gl-js#2268
The current work around is creating an arbitrary delay to ensure map has loaded styles before adding the layers which may fail with extreme slow internet speeds.

@kahama94 kahama94 changed the title Fix map adding layers before style loads Load layers after map style is loaded Jun 10, 2019
@kahama94 kahama94 added bug Something isn't working help wanted Extra attention is needed labels Jun 10, 2019
@cKellyDesign cKellyDesign self-assigned this Jun 18, 2019
@cKellyDesign
Copy link
Contributor

@moshthepitt @kahama94 What are the reproduction steps for this? I've removed the setTimeout locally and only get errors if I navigate off the MapView page (thus unmounting it) before the styles have a chance to load. In Gisdia-React.Map we're subscribing to the map load event, but we're calling setState in the callback which is the source of the error when the component is, in fact, no longer mounted. This particular quirk should be fixed with this PR.

Screen Shot 2019-06-18 at 1 35 57 PM

Are there specific steps I should take to reproduce this issue? Is there are a particular layer / focus area?

@kahama94
Copy link
Contributor Author

kahama94 commented Jun 19, 2019

@cKellyDesign Sorry I never included some documentation to replicate the issue. this PR fixes the issue style not loading but introduces another issue where if some one selects a map and goes back before map loading when you go back again to the map the map component unmounts.
steps to replicate the issue:

  • checkout gisida to branch 1.2.5 and gisida-react to branch 1.2.3 (this is what is being used currently on reveal)

  • on gisida-react 1.2.3 cherry pick the last commit on 111-unmounting-race git cherry-pick 75e2887616de8d2932258fbc995a640d1b7de82c

  • yarn link gisida and gisida-react to reveal then yarn start

  • select any map
    Screenshot from 2019-06-19 11-35-16
    and before the map fully loads go back to the page above then select the same map you chose.
    when given time to load the map loads fine but when some one initializes the map then leaves when coming back the map component unmounts. The setTimeOut on the other hand works fine but with slow internet where style loading takes longer than the arbitrary delay we have set the style not loading will recur

@cKellyDesign
Copy link
Contributor

cKellyDesign commented Jun 19, 2019

Oh boy, thanks @kahama94 I've been able to reproduce what you're describing and what your screencast shows. It looks like the Gisida-React.Map component is being unmounted because the GisidaWrapper internal state of doRenderMap is being set to false in componentWillRecieveProps after it's being set to true in initMap. (Thus re-running the render function and failing this conditional)

Screen Shot 2019-06-19 at 8 23 09 AM

cc @moshthepitt

@cKellyDesign
Copy link
Contributor

Is there a reason the two conditionals in GisidaWrapper.componentWillRecieveProps are separated instead of being and else if? Should the first be more specific to check for tasks? It's hard to say exactly but it feels like a race conditional between these two setStates and subsequent callbacks / setStates.

@cKellyDesign
Copy link
Contributor

@kahama94 @moshthepitt are either of the errors described above breaking the page?

If onaio/gisida-react#112 looks good then I can merge / tag a new gisida-react release.

@cKellyDesign
Copy link
Contributor

cKellyDesign commented Jun 24, 2019

@moshthepitt @kahama94 - This may be addressed at 4ff5266 on PR #133, I still get errors about unmounting if I navigate away before the style is done loading, but at least it's just a warning and isn't breaking the page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Gisida has pr help wanted Extra attention is needed Reveal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants