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

Fix Layer.addTo #1229

Merged
merged 16 commits into from
Jan 14, 2019
Merged

Fix Layer.addTo #1229

merged 16 commits into from
Jan 14, 2019

Conversation

VictorVelarde
Copy link
Contributor

@VictorVelarde VictorVelarde commented Jan 11, 2019

Closes #1228

1. Relevant facts

Some facts about this problem:

  1. It manifests just with MGL v0.52 (not in v0.50 and v0.51)
  2. It only has appeared in the editor.html, not in any of the examples (public / debug)
  3. The editor defines dinamically the Map's style (map.setStyle(selectedBasemap) vs the examples defining it usually within map's constructor options, with something like const map = new mapboxgl.Map({ style: carto.basemaps.positron ...
  4. The editor's code, regarding to map & layer loading is a bit convoluted when calling to layer.addTo(map, "watername_ocean").
  5. The common way to add layers to MGL map is:
    map.on('load', () => {
         map.addLayer(yourLayer, beforeLayerID?);
    });
  6. ... But in CARTO VL there is a convenient method to add a layer to the map, with just Layer.addTo(map, beforeLayerID). This is its current implementation:
    addTo (map, beforeLayerID) {
        const STYLE_ERROR_REGEX = /Style is not done loading/;
    
        const addTheLayer = () => { map.addLayer(this, beforeLayerID); };
        try {
            addTheLayer();
            // Note: map.isStyleLoaded() has been tested here without success
        } catch (error) {
            if (!STYLE_ERROR_REGEX.test(error)) {
                throw new CartoRuntimeError(`Error adding layer to map: ${error}`);
            }
    
            map.on('load', addTheLayer);
        }
    }
  7. So as we can see, that method has a fallback inside, just in case 'the map' is not ready yet (still loading'), to wrap the more common map.on('load', addTheLayer);... and that looks exactly like the situation here, so...

2. What's happening?

Why isn't that catch working?
The error raising from the first addLayer is not been caught. The try & catch doesn't work here, as the error is an Event, not a common thrown Error (see https://www.mapbox.com/mapbox-gl-js/api/#map.event:error). So there is no way to catch it like that, and that looks like the error in the console. Moreover, the error 'message' doesn't correspond in this case with current regex.

So, has v0.52 changed the way to raise the errors here... ? Yes, it has in this particular case!. It looks like it was not evented before. See: mapbox/mapbox-gl-js#7539

There is also a new warning, which corresponds to these changes: mapbox/mapbox-gl-js#7562 but it doesn't look harmful.

And why is the layer eventually loading?
The editor code has two calls to addLayer, and the second one seems succesful, as the basemap is then ready. To clean that is something we should address in #1204, so it is out of scope here.

3. Fix

Make Layer.addTo able to deal with common error (as before) and evented error (new in MGL v0.52)

@VictorVelarde VictorVelarde self-assigned this Jan 11, 2019
@VictorVelarde VictorVelarde changed the title Temporary set MGL version to v0.50.0 in the Editor Fix Layer.addTo Jan 11, 2019
@VictorVelarde
Copy link
Contributor Author

Note: all failing test 🔝 were test:user failing tests. In all those commits, the corresponding test:user:watchc passed all tests succesfully. The "fix" has been to test for the spy call without any surrounding map or layer .on events

@VictorVelarde VictorVelarde merged commit 1e27035 into master Jan 14, 2019
@VictorVelarde VictorVelarde deleted the 1228-fix-add-layer branch January 14, 2019 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant