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" event not firing #6076

Closed
stevage opened this issue Jan 28, 2018 · 17 comments
Closed

"load" event not firing #6076

stevage opened this issue Jan 28, 2018 · 17 comments

Comments

@stevage
Copy link
Contributor

stevage commented Jan 28, 2018

mapbox-gl-js version: 0.43.1 dev

I'm having an issue where sometimes the map's "load" event is not firing. In response to a user event, I'm adding and removing several layers at once, so to avoid a "Style is not done loading" error, I use this code:

map.onLoad(() => mappingBoundaries.activateBoundary(map, window.activeBoundarySet));

where map.onLoad is:

    map.onLoad = function(cb) {
        if  (map.loaded()) {
            cb();
        } else {
            map.once('load', () => {
                console.log('On loaded!');
                cb();
            });
        }
    };

Mostly this works as expected. But in this particular case, for whatever reason, the event never fires (so the "On loaded!" message never shows, and the function never gets called). There are no console messages.

It would be pretty hard to create a repro case from this. So I'm asking:

  1. Is there a more reliable way to achieve what I'm trying to do?
  2. Could functions like addLayer() please, please abstract away this monitoring of whether the style is ready or not? It's my biggest annoyance working with Mapbox-GL-JS on a daily basis. I would love addLayer() to just...add the layer. Now or later, I don't care, whenever the style is ready - just don't give me an error message, and don't require me to implement two different code paths to handle a state property I'm not very interested in.
@stevage
Copy link
Contributor Author

stevage commented Jan 28, 2018

Hmm, using map.once('styledata', ...) and map.isStyleLoaded() seems to be working reliably (in this specific case). Is that the right answer?

@stevage
Copy link
Contributor Author

stevage commented Jan 28, 2018

Hmm, actually no. If the user switches layers rapidly (that is, I call my onLoad function twice within say, half a second), the second event doesn't seem to fire.

@lbud
Copy link
Contributor

lbud commented Feb 5, 2018

@stevage it's hard to diagnose your problem without an example that reproduces it, but I'd suspect if you're finding a situation where your load listener is never being invoked, there's probably a race condition where the listener isn't attached before the load event gets fired. A jsfiddle reproducing this issue could confirm that hunch or otherwise help us diagnose exactly what it is.

As to your second question, it's unlikely that we'll implement a system to queue mutations for when the style is "ready" for whatever that mutation is, as doing so would force us to make assumptions about what the developer wants to do and in what order. (You alluded to this, sort of, in

Now or later, I don't care

…because what if for some application-specific reason you did care? :) )

@stevage
Copy link
Contributor Author

stevage commented Feb 6, 2018

it's hard to diagnose your problem without an example that reproduces it

Yeah, I know. Making repros isn't always practical, though, for various reasons. I do try.

As to your second question, it's unlikely that we'll implement a system to queue mutations for when the style is "ready" for whatever that mutation is, as doing so would force us to make assumptions about what the developer wants to do and in what order.

It's funny though, I end up using this onLoad() function in virtually every Mapbox-GL-JS project I work on. I don't see what the alternative is. Using .on("load"...) almost guarantees a race condition, between the code path that renders the map, and the code path which is attaching this handler.

A common example for me is:

  1. Load and render base map
  2. Meanwhile, fetch a CSV file.
  3. When map and CSV file are both ready, display points on the map.

It's not safe to use .on("load", because the map might win the race. It's not safe to just use map.addLayer() because the CSV path might win the race. The only viable option is to test whether the map's ready, and if not, attach a load event. I'm just really surprised that this isn't everybody else's experience too.

…because what if for some application-specific reason you did care? :) )

Then I'd revert to using these more specific handlers. Currently, addLayer() either adds data to the map, or throws an error. I guess I'm suggesting that addLayer() would either add data to a map, or queue adding data to a map. I don't think the user is ever worse off under that scenario.

@stevage
Copy link
Contributor Author

stevage commented Feb 15, 2018

Ok, I've made a JSBin which I hope demonstrates that there is either a bug, or the documentation is unclear, or...something. :)

https://jsbin.com/yahirajuxe/edit?html,console,output

I'm starting with the (possibly false) assumption that:

  • Either map.loaded() is true, or "load" will eventually fire.
  • Either map.isStyleLoaded() is true, or "styledata" will eventually fire.
  • Either map.areTilesLoaded() is true, or "data" will eventually fire.

After 5 seconds, this map does the following:

function wait(event, n) {
log('Wait for ' + event + ' ' + n + '...');
map.once(event, () => log('...' + event + ' ' + n));
}

function report() {
    log(map.loaded() ? 'Loaded': 'NotLoaded', map.isStyleLoaded() ? 'StyleLoaded':'StyleNotLoaded', map.areTilesLoaded()?'TilesLoaded':'TilesNotLoaded',Math.random());
    if (!map.loaded()) {
        wait('load', n++);
    }
    if (!map.isStyleLoaded()) {
        wait('styledata', n++);
    }
    if (!map.areTilesLoaded()) {
        wait('data', n++);
        wait('sourcedata', n++);
    }
}

The results (on my computer):

"(styledata) 0"
"(styledata) 1"
"(styledata) 2"
"(load) 3"
"Loaded StyleLoaded TilesLoaded 0.013149924653681477"
"Loaded StyleLoaded TilesLoaded 0.3974862547080237"
"Loaded StyleLoaded TilesLoaded 0.07838910786339992"
"NotLoaded StyleNotLoaded TilesNotLoaded 0.29928696827228873"
"Wait for load 0..."
"Wait for styledata 1..."
"Wait for data 2..."
"Wait for sourcedata 3..."
"NotLoaded StyleNotLoaded TilesNotLoaded 0.7180447813438798"
"Wait for load 4..."
"Wait for styledata 5..."
"Wait for data 6..."
"Wait for sourcedata 7..."
"...sourcedata 3"
"...sourcedata 7"
"...data 2"
"...data 6"

In other words: load and styledata are fired when the style initially loads, but then never again - even though isLoaded() and isStyleLoaded() are reporting false as the camera zooms in.

Just to be clear: this is not just academic interest. I'm having a lot of trouble with a site now in production with this scenario:

  1. User zooms in
  2. User adds a layer (without waiting for the map to fully paint).

Any reliable workaround would be great.

@stevage
Copy link
Contributor Author

stevage commented Feb 15, 2018

I've made another version, focusing on whether you can use loaded(), isStyleLoaded() and areTilesLoaded() to know whether it's safe to call addLayer(): https://jsbin.com/zijujoxipo/edit?html,console,output

In this version, every time you see something like NotLoaded StyleNotLoaded TilesNotLoaded 0.5434786914344556 in the log, we're also calling addLayer().

Output (edited):

NotLoaded StyleNotLoaded TilesLoaded 0.4564971102699287
NotLoaded StyleNotLoaded TilesLoaded 0.4603425978930812
Error: Style is not done loading
NotLoaded StyleNotLoaded TilesLoaded 0.19410982374725794
Error: Style is not done loading
NotLoaded StyleNotLoaded TilesNotLoaded 0.027212040566639506
Loaded StyleLoaded TilesLoaded 0.1304981385983136
Loaded StyleLoaded TilesLoaded 0.1885207684258149
Loaded StyleLoaded TilesLoaded 0.29673921585506047
NotLoaded StyleNotLoaded TilesNotLoaded 0.4405454076552441
NotLoaded StyleNotLoaded TilesNotLoaded 0.9542045293397484

Notes:

  • After the first "NotLoaded StyleNotLoaded TilesLoaded", addLayer() succeeds (no error message).
  • After the exact same state, addLayer() fails (twice).
  • Then in the state "NotLoaded StyleNotLoaded TilesNotLoaded", it succeeds.
  • Finally, after some successes, again in the "NotLoaded StyleNotLoaded TilesNotLoaded" state, it suceeds twice.

My conclusions:

  • There is no way to use those three functions to determine whether a call to addLayer() will succeed. (Except if all three are true it is safe.).
  • There is no obvious (to me) way to use a combination of those functions, plus the load, styledata and sourcedata events to reliably add a layer, no matter what state the map is currently in.

My best guess at a workaround at this point would involve some kind of chaining: wait, check whether the map is in a good state, and if not, wait again. Or maybe, just listen out for the "error" message, but it's probably really hard to pass state through it.

@stevage
Copy link
Contributor Author

stevage commented Feb 15, 2018

FWIW, this onLoad function seems to be more reliable for me. Basically, avoid listening for "load" or "styledata" where possible.

    map.onLoad = function(cb) {
        if  (map.loaded()) {
            cb();
        } else if (!map.areTilesLoaded()) {
            map.once('data', cb);
        } else if (!map.isStyleLoaded()) {
            map.once('styledata', cb);
        } else {
            console.log("Map is not ready but is not not-ready either.");
        }
        return map;
    };

Bonus: the initial page load is faster, as we can add the data after the initial tiles have loaded, but before the first map paint is complete.

@jfirebaugh
Copy link
Contributor

Either map.loaded() is true, or "load" will eventually fire.

This isn't correct. map.loaded() returns true if the map is currently complete, i.e. all resources necessary to display the current viewport contents have been downloaded and processed. It can go from true to false to true and so on an unlimited number of times in a single session with a single style.

load fires only after the first visually complete rendering of the map has occurred.

It sounds like you're looking for #1715.

@jfirebaugh
Copy link
Contributor

Reading back to try to glean your concrete use case:

  1. Load and render base map
  2. Meanwhile, fetch a CSV file.
  3. When map and CSV file are both ready, display points on the map.

What you need to do here is not specifically GL JS related, but a general concurrent programming problem: how do you do something only when two asynchronous tasks are both complete?

One approach is something like:

  1. Initialize two booleans loadedMap and loadedCSV to false.
  2. Attach a load event handler for the map that sets loadedMap to true and checks if loadedCSV is also true. If so, do your thing. If not, do nothing.
  3. Attach a load event handler for the CSV file that sets loadedCSV to true and checks if loadedMap is also true. If so, do your thing. If not, do nothing.

@stevage
Copy link
Contributor Author

stevage commented Feb 16, 2018

Thanks, but I don't believe I am looking for #1715. Use case there seem to be around notifying the user that data is still loading, or knowing when it's safe to start querying data in the map (because all the data is there).

My use case is really simple: a guaranteed way to call addLayer() that will not fail because the map is "not ready".

One approach is something like:

Initialize two booleans loadedMap and loadedCSV to false.
Attach a load event handler for the map that sets loadedMap to true and checks if loadedCSV is also true. If so, do your thing. If not, do nothing.
Attach a load event handler for the CSV file that sets loadedCSV to true and checks if loadedMap is also true. If so, do your thing. If not, do nothing.

Yes, thanks - I don't need help with the concurrent programming bit :) The problem (as I explained in quite a lot of detail here ) is that this part of your solution:

Attach a load event handler for the map that sets loadedMap to true

...doesn't work. As you note, it only fires the first time a map loads, and never again. So in this scenario:

  1. Load base map.
  2. ...time passes...
  3. Add a layer
  4. Add another layer

There is seemingly no way to know when it's safe to call step 4, because you can't wait for load after step 3, because it won't happen.

@jfirebaugh
Copy link
Contributor

  1. Add a layer
  2. Add another layer

In this scenario, why do you need to wait for 3 to complete to do 4?

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Feb 16, 2018

Another guess about what you mean: you have two or more independent things that need to happen once the map is loaded. (But they don't necessarily need to be ordered with respect to each other.)

var ready = false;
map.on('load', function () { ready = true; });

function onReady(cb) {
    if  (ready) {
        cb();
    } else {
        map.on('load', cb);
    }
}

onReady(function () { map.addLayer(...) });
onReady(function () { map.addLayer(...) });

If that's not it either, please please please provide a JSFiddle. I'm begging you! 😅

@stevage
Copy link
Contributor Author

stevage commented Feb 16, 2018

Heh. I can't provide an actual JSfiddle because it's a confidential site for a client, full of confidential data. So the best I can do is to provide a simulation demonstrating the issue, which is exactly what I did here and here.

Here's how this app works:

  1. Load basemap.
  2. Add a polygon overlay.
  3. Also fetch a CSV file, and when that arrives, add that as a set of points.
  4. Some time later, the user chooses to enable a different overlay.

Now, I was using if (!map.loaded) { on('load'... to handle 2, 3 and 4. I have now learnt from this thread that that was never going to work for 4. So that caused the problem I described, where if the user zooms in, and then adds an overlay before the map has fully painted, then the layer never shows up: the map is not loaded() (it's still painting), yet the load event never fires because, by a different definition of "loaded", the map has loaded (which it only ever does once).

Ok, so to answer your question:

In this scenario, why do you need to wait for 3 to complete to do 4?

Because: all I want is a reliable way to add layers, no matter what the current state of the map is. I guess you know that I don't need to wait for one addLayer() to complete before issuing another addLayer(). I didn't know that. All I know is that sometimes when I call addLayer() it fails with an error message about the "style not done loading", and I haven't been able to find a way to work out whether addLayer() will fail or not.

So, I guess, I'm begging you: tell me what the body of this function should be:

/* 
Contract: no matter what the current state of the map is (still loading style, or fetching tiles, or we just added 3 layers, or the user is zooming...), this layer will definitely eventually get added to the map.
*/
function safelyAddLayer(map, layer) {
...
}

My current best guess is:

function safelyAddLayer(map, layer) {
    if (map.loaded()) {
        map.addLayer(layer);
    } else {
        map.once('data', () => map.addLayer(layer));
    }
}

but honestly I don't know if there are race conditions, or particular scenarios where it will fail.

@pathmapper
Copy link
Contributor

In this scenario, why do you need to wait for 3 to complete to do 4?

Having a use case where I filter 4 based on queryRenderedFeatures of 3.

Currently I use setTimeout to wait until 3 is rendered.

@stevage did you notice #4222 (comment)? Maybe this helps in your use case...

@mollymerp
Copy link
Contributor

(deleted previous comment bc I think @jfirebaugh had the right response in #6076 (comment))

@jfirebaugh
Copy link
Contributor

@stevage
Copy link
Contributor Author

stevage commented Feb 25, 2018

Ok, thank you. If you compare your solution to what I wrote in the original problem description, it boils down to the fact that .loaded() is basically not useful for determining whether one can safely add a layer, and the user is responsible for adding a .once("load", () => map.reallyLoaded = true listener in order to be able to safely add layers later.

The thing that confuses me most about this whole thread is why no one else seems to think this is much of a problem :)

The actual version I'm now using in production:

map.onLoad = function(cb) {
        function cb2() {
            map.actuallyLoaded = true;
            cb();
        }
        if  (map.loaded() || map.actuallyLoaded) {
            cb2();
        } else if (!map.areTilesLoaded()) {
            map.once('data', cb2);
        } else if (!map.isStyleLoaded()) {
            map.once('styledata', cb2);
        } else {
            console.log("Map is not ready but is not not-ready either.");
            cb2();
        }
        return map;
    };

This version (listening to 'styledata', not 'load') also seems to get to first page paint faster. This way, the extra data layer gets rendered a second or two before the basemap arrives. If I listen to 'load', the basemap paints (which takes a couple of seconds of vector tile fetching - slow internet in Australia), and then finally the data layer (which is what my users care about) paints.

It might be worth improving the documentation of map#loaded() to spell out what "fully loaded" means (in terms of what other actions are permissible). Similarly, it might be worth mentioning in addLayer() what the required state of the map is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants