From 87c9f7f295266841f4a070bddee64867fd719667 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Fri, 11 Jan 2019 18:14:53 +0100 Subject: [PATCH 01/16] Temporary set MGL version to v0.50.0 in the Editor --- examples/editor/index.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/editor/index.html b/examples/editor/index.html index b281dc71d..1d12384c4 100644 --- a/examples/editor/index.html +++ b/examples/editor/index.html @@ -7,8 +7,8 @@ CARTO VL EDITOR - - + + From 5f4a1ee9a22fd1aaefce140857c6463e12e219de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Fri, 11 Jan 2019 20:59:32 +0100 Subject: [PATCH 02/16] Refactor util for easier reuse in integration tests --- test/integration/util.js | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/test/integration/util.js b/test/integration/util.js index 122be1d8b..61085ce2f 100644 --- a/test/integration/util.js +++ b/test/integration/util.js @@ -4,6 +4,19 @@ import { projectToWebMercator, WM_2R } from '../../src/utils/util'; const mapSize = 600; export function createMap (name, size) { + const div = createMapDivHolder(name, size); + + const map = new mapboxgl.Map({ + container: name, + style: { version: 8, sources: {}, layers: [] }, + center: [0, 0], + zoom: 0 + }); + + return { div, map }; +} + +export function createMapDivHolder (name, size) { size = size || mapSize; const div = document.createElement('div'); @@ -16,14 +29,7 @@ export function createMap (name, size) { document.body.style.padding = '0'; document.body.appendChild(div); - const map = new mapboxgl.Map({ - container: name, - style: { version: 8, sources: {}, layers: [] }, - center: [0, 0], - zoom: 0 - }); - - return { div, map }; + return div; } function _mouseParamsFromCoords (coordinates) { From 24bcee5c52d566dbc0ac468ff091e11d5a4513f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Fri, 11 Jan 2019 21:05:00 +0100 Subject: [PATCH 03/16] Consider evented error from the map when adding a Layer --- src/Layer.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Layer.js b/src/Layer.js index 98e28d53a..1478b093a 100644 --- a/src/Layer.js +++ b/src/Layer.js @@ -157,21 +157,21 @@ export default class Layer { * @api */ addTo (map, beforeLayerID) { - const STYLE_ERROR_REGEX = /Style is not done loading/; - - const addTheLayer = () => { map.addLayer(this, beforeLayerID); }; + // Manage errors, whether they are an Evented Error or a common Error try { - addTheLayer(); - // Note: map.isStyleLoaded() has been tested here without success + map.once('error', () => { this._waitForMapToLoad(map, beforeLayerID); }); + map.addLayer(this, beforeLayerID); } catch (error) { - if (!STYLE_ERROR_REGEX.test(error)) { - throw new CartoRuntimeError(`Error adding layer to map: ${error}`); - } - - map.on('load', addTheLayer); + this._waitForMapToLoad(map, beforeLayerID); } } + _waitForMapToLoad (map, beforeLayerID) { + map.once('load', () => { + map.addLayer(this, beforeLayerID); + }); + } + /** * Remove this layer from the map. It should be called after the layer is loaded. Otherwise, it will not delete the layer. * From 80b4d76993786478b4e90ac4294bcc3628ec1062 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Fri, 11 Jan 2019 21:05:59 +0100 Subject: [PATCH 04/16] Add test to verify that Layer waits for map to load, also in v0.52 --- test/integration/user/test/layer.test.js | 332 ++++++++++++----------- 1 file changed, 179 insertions(+), 153 deletions(-) diff --git a/test/integration/user/test/layer.test.js b/test/integration/user/test/layer.test.js index faf950054..fa5b1a5db 100644 --- a/test/integration/user/test/layer.test.js +++ b/test/integration/user/test/layer.test.js @@ -1,4 +1,5 @@ import carto from '../../../../src/'; +import mapboxgl from 'mapbox-gl'; import * as util from '../../util'; import { mat4 } from 'gl-matrix'; @@ -14,204 +15,229 @@ const featureData = { describe('Layer', () => { let div, map, source, viz, layer; - beforeEach(() => { - const setup = util.createMap('map'); - div = setup.div; - map = setup.map; - - source = new carto.source.GeoJSON(featureData); - viz = new carto.Viz(` - @myColor: red - color: @myColor - `); - layer = new carto.Layer('layer', source, viz); - layer._paintLayer = () => { }; - layer.addTo(map); - }); - - describe('.on', () => { - it('should fire a "loaded" event when ready', (done) => { - layer.on('loaded', done); + describe('when a map is ready', () => { + beforeEach(() => { + const setup = util.createMap('map'); + div = setup.div; + map = setup.map; + + source = new carto.source.GeoJSON(featureData); + viz = new carto.Viz(` + @myColor: red + color: @myColor + `); + layer = new carto.Layer('layer', source, viz); + layer._paintLayer = () => { }; + layer.addTo(map); }); - describe('.updated', () => { - function mockPrerenderHelpers () { - // auxiliary mocks for prerender - spyOn(layer, '_getZoom').and.callFake(() => 0); - spyOn(layer, '_getViewport').and.callFake(() => { }); - spyOn(layer, '_needRefresh').and.callFake(() => Promise.resolve()); - } - - function resetMocks () { - layer._getZoom.calls.reset(); - layer._getViewport.calls.reset(); - layer._needRefresh.calls.reset(); - } - - it('should fire an "updated" event when ready', (done) => { - layer.on('updated', done); + describe('.on', () => { + it('should fire a "loaded" event when ready', (done) => { + layer.on('loaded', done); }); - it('should fire an "updated" event only once when ready', (done) => { - let update = jasmine.createSpy('update'); - layer.on('updated', update); - layer.on('loaded', () => { - setTimeout(() => { - expect(update).toHaveBeenCalled(); - done(); - }, 0); + describe('.updated', () => { + function mockPrerenderHelpers () { + // auxiliary mocks for prerender + spyOn(layer, '_getZoom').and.callFake(() => 0); + spyOn(layer, '_getViewport').and.callFake(() => { }); + spyOn(layer, '_needRefresh').and.callFake(() => Promise.resolve()); + } + + function resetMocks () { + layer._getZoom.calls.reset(); + layer._getViewport.calls.reset(); + layer._needRefresh.calls.reset(); + } + + it('should fire an "updated" event when ready', (done) => { + layer.on('updated', done); }); - }); - - it('should fire an "updated" event when the source is updated', (done) => { - mockPrerenderHelpers(); - layer.on('loaded', async () => { - const newSource = new carto.source.GeoJSON(featureData); - spyOn(newSource, 'requestData').and.callFake(() => true); + it('should fire an "updated" event only once when ready', (done) => { let update = jasmine.createSpy('update'); layer.on('updated', update); + layer.on('loaded', () => { + setTimeout(() => { + expect(update).toHaveBeenCalled(); + done(); + }, 0); + }); + }); + + it('should fire an "updated" event when the source is updated', (done) => { + mockPrerenderHelpers(); + layer.on('loaded', async () => { + const newSource = new carto.source.GeoJSON(featureData); + spyOn(newSource, 'requestData').and.callFake(() => true); + + let update = jasmine.createSpy('update'); + layer.on('updated', update); - await layer.update(newSource); - const currentMatrix = layer.renderer.matrix; - layer.prerender(null, currentMatrix); + await layer.update(newSource); + const currentMatrix = layer.renderer.matrix; + layer.prerender(null, currentMatrix); - expect(update).toHaveBeenCalledTimes(1); - resetMocks(); - done(); + expect(update).toHaveBeenCalledTimes(1); + resetMocks(); + done(); + }); }); - }); - it('should fire a new "updated" after moving the map (even if not requiring new dataframes)', (done) => { - mockPrerenderHelpers(); + it('should fire a new "updated" after moving the map (even if not requiring new dataframes)', (done) => { + mockPrerenderHelpers(); - let numberOfUpdates = 0; - let update = () => { - numberOfUpdates++; + let numberOfUpdates = 0; + let update = () => { + numberOfUpdates++; - if (numberOfUpdates === 2) { - resetMocks(); - done(); // ...(b) we get a second update - return; - } - - // // Emulate a user interaction, like dragging the map, using the underlying matrix - const matrix = layer.renderer.matrix; - let newMatrix = mat4.create(); - mat4.translate(newMatrix, matrix, [10, 0, 0]); // 10 units translation in x - - // (a) as it is a geojson, no dataframesHaveChange, but we need a new update anyway... - layer.prerender(null, newMatrix); - }; - layer.on('updated', update); - }); + if (numberOfUpdates === 2) { + resetMocks(); + done(); // ...(b) we get a second update + return; + } - it('should fire an "updated" event when the viz is updated', (done) => { - mockPrerenderHelpers(); - layer.on('loaded', async () => { - let update = jasmine.createSpy('update'); + // // Emulate a user interaction, like dragging the map, using the underlying matrix + const matrix = layer.renderer.matrix; + let newMatrix = mat4.create(); + mat4.translate(newMatrix, matrix, [10, 0, 0]); // 10 units translation in x + + // (a) as it is a geojson, no dataframesHaveChange, but we need a new update anyway... + layer.prerender(null, newMatrix); + }; layer.on('updated', update); + }); + + it('should fire an "updated" event when the viz is updated', (done) => { + mockPrerenderHelpers(); + layer.on('loaded', async () => { + let update = jasmine.createSpy('update'); + layer.on('updated', update); - await layer.update(source, new carto.Viz('color: blue')); - const currentMatrix = layer.renderer.matrix; - layer.prerender(null, currentMatrix); + await layer.update(source, new carto.Viz('color: blue')); + const currentMatrix = layer.renderer.matrix; + layer.prerender(null, currentMatrix); - expect(update).toHaveBeenCalled(); - resetMocks(); - done(); + expect(update).toHaveBeenCalled(); + resetMocks(); + done(); + }); }); - }); - it('should fire an "updated" event when the viz is animated', async (done) => { - let update = jasmine.createSpy('update'); - layer.on('updated', update); + it('should fire an "updated" event when the viz is animated', async (done) => { + let update = jasmine.createSpy('update'); + layer.on('updated', update); - await layer.update(source, new carto.Viz('width: now()')); + await layer.update(source, new carto.Viz('width: now()')); - layer.on('loaded', () => { - layer.render(); - expect(update).toHaveBeenCalled(); + layer.on('loaded', () => { + layer.render(); + expect(update).toHaveBeenCalled(); - layer.render(); - expect(update).toHaveBeenCalled(); - done(); + layer.render(); + expect(update).toHaveBeenCalled(); + done(); + }); }); }); - }); - describe('.hide', () => { - it('should hide a visible layer', (done) => { - layer.on('loaded', () => { - expect(layer.visible).toBeTruthy(); - layer.hide(); - expect(layer.visible).toBeFalsy(); - done(); + describe('.hide', () => { + it('should hide a visible layer', (done) => { + layer.on('loaded', () => { + expect(layer.visible).toBeTruthy(); + layer.hide(); + expect(layer.visible).toBeFalsy(); + done(); + }); }); - }); - it('should trigger an update event', (done) => { - layer.on('loaded', () => { - layer.on('updated', done); - layer.hide(); + it('should trigger an update event', (done) => { + layer.on('loaded', () => { + layer.on('updated', done); + layer.hide(); + }); }); - }); - it('should not request source data', (done) => { - layer.on('loaded', () => { - const requestDataSourceSpy = spyOn(layer._source, 'requestData'); - layer.hide(); - const currentMatrix = layer.renderer.matrix; - layer.prerender(null, currentMatrix); + it('should not request source data', (done) => { + layer.on('loaded', () => { + const requestDataSourceSpy = spyOn(layer._source, 'requestData'); + layer.hide(); + const currentMatrix = layer.renderer.matrix; + layer.prerender(null, currentMatrix); - expect(requestDataSourceSpy).not.toHaveBeenCalled(); - done(); + expect(requestDataSourceSpy).not.toHaveBeenCalled(); + done(); + }); }); }); - }); - describe('.show', () => { - beforeEach(() => { - layer.on('loaded', () => { - layer.hide(); + describe('.show', () => { + beforeEach(() => { + layer.on('loaded', () => { + layer.hide(); + }); }); - }); - it('should show a hidden layer', (done) => { - layer.on('loaded', () => { - expect(layer.visible).toBeFalsy(); - layer.show(); - expect(layer.visible).toBeTruthy(); - done(); + it('should show a hidden layer', (done) => { + layer.on('loaded', () => { + expect(layer.visible).toBeFalsy(); + layer.show(); + expect(layer.visible).toBeTruthy(); + done(); + }); + }); + + it('should request source data', (done) => { + layer.on('loaded', () => { + const requestDataFn = layer._source.requestData.bind(layer._source); + let requestDataCalled = false; + layer._source.requestData = (...args) => { + requestDataCalled = true; + return requestDataFn(...args); + }; + // spyOn(layer._source, 'requestData'); + layer.hide(); + layer.show(); + layer.prerender(undefined, mat4.identity([])); + layer._matrix = mat4.identity([]); + layer._matrix[0] = 2; + layer.prerender(undefined, mat4.identity([])); + + expect(requestDataCalled).toBeTruthy(); + done(); + }); }); }); + }); - it('should request source data', (done) => { + describe('.blendToViz', () => { + it('should resolve the Promise with a valid viz', (done) => { layer.on('loaded', () => { - const requestDataFn = layer._source.requestData.bind(layer._source); - let requestDataCalled = false; - layer._source.requestData = (...args) => { - requestDataCalled = true; - return requestDataFn(...args); - }; - // spyOn(layer._source, 'requestData'); - layer.hide(); - layer.show(); - layer.prerender(undefined, mat4.identity([])); - layer._matrix = mat4.identity([]); - layer._matrix[0] = 2; - layer.prerender(undefined, mat4.identity([])); - - expect(requestDataCalled).toBeTruthy(); - done(); + layer.blendToViz(new carto.Viz('color: blue')).then(done); }); }); }); }); - describe('.blendToViz', () => { - it('should resolve the Promise with a valid viz', (done) => { - layer.on('loaded', () => { - layer.blendToViz(new carto.Viz('color: blue')).then(done); + describe('when map is not ready', () => { + it('should wait for it load', (done) => { + const name = 'map'; + div = util.createMapDivHolder(name); + map = new mapboxgl.Map({ + container: name, + center: [0, 0], + zoom: 0 + }); + const source = new carto.source.GeoJSON(featureData); + const layer = new carto.Layer('layer', source, new carto.Viz()); + + const waitForMapToLoad = spyOn(layer, '_waitForMapToLoad').and.callFake(() => { }); + layer.addTo(map, 'watername_ocean'); // << adding before setting basemap + map.setStyle(carto.basemaps.voyager); + + map.on('load', () => { + expect(waitForMapToLoad).toHaveBeenCalled(); + done(); }); }); }); From d8cebd808b19e04fd453caea58632cfcb6c1db71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 10:16:47 +0100 Subject: [PATCH 05/16] Manage errors, evented or common type --- src/Layer.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/Layer.js b/src/Layer.js index 1478b093a..99188dce2 100644 --- a/src/Layer.js +++ b/src/Layer.js @@ -159,15 +159,22 @@ export default class Layer { addTo (map, beforeLayerID) { // Manage errors, whether they are an Evented Error or a common Error try { - map.once('error', () => { this._waitForMapToLoad(map, beforeLayerID); }); + map.once('error', (data) => { + console.warn(data.error.message); + this._waitForMapToLoad(map, beforeLayerID); + }); map.addLayer(this, beforeLayerID); } catch (error) { + const STYLE_ERROR_REGEX = /Style is not done loading/; + if (!STYLE_ERROR_REGEX.test(error)) { + throw new CartoRuntimeError(`Error adding layer to map: ${error}`); + } this._waitForMapToLoad(map, beforeLayerID); } } _waitForMapToLoad (map, beforeLayerID) { - map.once('load', () => { + map.on('load', () => { map.addLayer(this, beforeLayerID); }); } From e64dd22b88f8ccd54bc0af9d13520ffdcef85512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 10:17:10 +0100 Subject: [PATCH 06/16] Fix map mocks in tests --- test/unit/layer.test.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/unit/layer.test.js b/test/unit/layer.test.js index d1efaa274..a1bcea12e 100644 --- a/test/unit/layer.test.js +++ b/test/unit/layer.test.js @@ -146,7 +146,8 @@ describe('api/layer', () => { it('should call onMapLoaded when the map is loaded', () => { const layer = new Layer('layer0', source, viz); const mapMock = { - addLayer: jasmine.createSpy('addLayer') + addLayer: jasmine.createSpy('addLayer'), + once: () => { } }; layer.addTo(mapMock, 'beforeLayer'); expect(mapMock.addLayer).toHaveBeenCalledWith(layer, 'beforeLayer'); @@ -159,7 +160,8 @@ describe('api/layer', () => { layer.onAdd = (map) => { layer.map = map; }; const mapMock = { addLayer: (layer) => { layer.onAdd(mapMock); }, - removeLayer: jasmine.createSpy('removeLayer') + removeLayer: jasmine.createSpy('removeLayer'), + once: () => { } }; layer.addTo(mapMock); layer.remove(); From 00af08edca72d175787e9ec5fcc64b827acc5060 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 10:18:23 +0100 Subject: [PATCH 07/16] Back to v0.52 in editor, after fixing evented error in layer.addTo --- examples/editor/index.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/editor/index.html b/examples/editor/index.html index 1d12384c4..b281dc71d 100644 --- a/examples/editor/index.html +++ b/examples/editor/index.html @@ -7,8 +7,8 @@ CARTO VL EDITOR - - + + From 9bf7271b6d81591387481a38f1cd2ac8ff761596 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 10:52:05 +0100 Subject: [PATCH 08/16] Add render test to check the waitForMapToLoad --- .../regression/layer-addTo/reference.png | Bin 0 -> 2027 bytes .../regression/layer-addTo/scenario.js | 30 ++++++++++++++++++ test/integration/user/test/layer.test.js | 4 ++- 3 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 test/integration/render/scenarios/regression/layer-addTo/reference.png create mode 100644 test/integration/render/scenarios/regression/layer-addTo/scenario.js diff --git a/test/integration/render/scenarios/regression/layer-addTo/reference.png b/test/integration/render/scenarios/regression/layer-addTo/reference.png new file mode 100644 index 0000000000000000000000000000000000000000..4b2e08591a4e129fb14590518ee545d175e88d11 GIT binary patch literal 2027 zcmeAS@N?(olHy`uVBq!ia0y~yV4MKNIvi|3k+<8Q9%5i%-{k4y7*a9k?QP$l&~nM+ zAK7PGyBg2*>JI!NI{G$jGRmq`=_9!qU*uz#t$fz@Ws$|117F*?ZcR`Tq0P$lGpQ-8Zi)HK@EU^XjXp?VqNtT2=My zLF2EV`SWLAsg>XP?$@tdveK`dxy7zL6jp!r@b3OK;m3}tm8x%9XqdL^Twk@BskqGwZ(_$e!Y9VF7WyOzjI&x`*X@yX@HeUtXc* zk3Jpti$7cWM*Zv4+jimoHoMnvoxA-1hnm_CH$47+zIZo$PHn>J@Yni#^LXp(VzYlQ z@ReI+to!xsoXGBJ`m4G9Rd@OweR?tQV6{i{~JbMck2 z4BH-GrFW^#l9?H#XxEPNAFG~=&$|;2(!Xz}arAA@Tp@vF>m}bmdb9V>_s_HE|Gj%h z-N%{f&=!-6udlvXDE@j@>GrsL(i{|EYI$htJi??QAX9Gb`}{BLyW{O;e?kqC|J}W_J+rv@;g^})_a6Hlb=lVZ zn$3>X*Gt~a?z&zfe`nLym-D2nAK$xI)BF9)%xC+2XD|Qr+x*$wW6=>l)!fg{uCKmc zztC=1MPYAG-!=aEH$sp5&!2T=?*I4oDt|s~y?*uj`x^n>;(BZBdwZ@N-d!K6etxgn tO6!O9s=xx#hIzDL9IYKdRpi8a#s@*pl9q-KmjNqr22WQ%mvv4FO#t;hvAzHR literal 0 HcmV?d00001 diff --git a/test/integration/render/scenarios/regression/layer-addTo/scenario.js b/test/integration/render/scenarios/regression/layer-addTo/scenario.js new file mode 100644 index 000000000..440bc2ba2 --- /dev/null +++ b/test/integration/render/scenarios/regression/layer-addTo/scenario.js @@ -0,0 +1,30 @@ +const map = new mapboxgl.Map({ + container: 'map', + center: [0, 0], + style: { version: 8, sources: {}, layers: [] }, + zoom: 0 +}); + +const source = new carto.source.GeoJSON(sources['points']); +const viz = new carto.Viz(); +const layer = new carto.Layer('layer', source, viz); + +layer.addTo(map, 'background'); + +// Setting map's style after trying to add the layer... +const style = { + version: 8, + sources: {}, + layers: [{ + id: 'background', + type: 'background', + paint: { + 'background-color': 'black' + } + }] +}; +map.setStyle(style); + +layer.on('loaded', () => { + window.loaded = true; +}); diff --git a/test/integration/user/test/layer.test.js b/test/integration/user/test/layer.test.js index fa5b1a5db..8d2963868 100644 --- a/test/integration/user/test/layer.test.js +++ b/test/integration/user/test/layer.test.js @@ -226,8 +226,10 @@ describe('Layer', () => { map = new mapboxgl.Map({ container: name, center: [0, 0], - zoom: 0 + zoom: 0, + style: { version: 8, sources: {}, layers: [] } }); + const source = new carto.source.GeoJSON(featureData); const layer = new carto.Layer('layer', source, new carto.Viz()); From 7243b8b6dca8bb4064d7aa1d26370c13446f9ba0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 11:48:42 +0100 Subject: [PATCH 09/16] Add dummy once method to CartoMap --- test/CartoMap.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/CartoMap.js b/test/CartoMap.js index 6d2bdf9de..d80d1743e 100644 --- a/test/CartoMap.js +++ b/test/CartoMap.js @@ -42,6 +42,10 @@ class CartoMap { // eslint-disable-line no-unused-vars } } + once () { + + } + addLayer (layer, beforeLayerID) { if (!this._layers.has(layer)) { this._layers.add(layer); From 8ba29901f5a69b21225a74e4b87f343b7e36b8fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 13:14:30 +0100 Subject: [PATCH 10/16] Allowing map with empty style at the beginning --- src/Layer.js | 3 ++- test/integration/user/test/layer.test.js | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Layer.js b/src/Layer.js index 99188dce2..6846e5a38 100644 --- a/src/Layer.js +++ b/src/Layer.js @@ -166,7 +166,8 @@ export default class Layer { map.addLayer(this, beforeLayerID); } catch (error) { const STYLE_ERROR_REGEX = /Style is not done loading/; - if (!STYLE_ERROR_REGEX.test(error)) { + const NO_STYLE_AT_ALL = /Cannot read property 'addLayer' of undefined/; + if (!(STYLE_ERROR_REGEX.test(error) || NO_STYLE_AT_ALL.test(error))) { throw new CartoRuntimeError(`Error adding layer to map: ${error}`); } this._waitForMapToLoad(map, beforeLayerID); diff --git a/test/integration/user/test/layer.test.js b/test/integration/user/test/layer.test.js index 8d2963868..8450872b6 100644 --- a/test/integration/user/test/layer.test.js +++ b/test/integration/user/test/layer.test.js @@ -226,14 +226,13 @@ describe('Layer', () => { map = new mapboxgl.Map({ container: name, center: [0, 0], - zoom: 0, - style: { version: 8, sources: {}, layers: [] } + zoom: 0 }); const source = new carto.source.GeoJSON(featureData); const layer = new carto.Layer('layer', source, new carto.Viz()); - const waitForMapToLoad = spyOn(layer, '_waitForMapToLoad').and.callFake(() => { }); + const waitForMapToLoad = spyOn(layer, '_waitForMapToLoad').and.callThrough(); layer.addTo(map, 'watername_ocean'); // << adding before setting basemap map.setStyle(carto.basemaps.voyager); From 52b79e4e6d00f2cc22392522bde02e944016b520 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 13:43:05 +0100 Subject: [PATCH 11/16] Test on layer.loaded --- test/integration/user/test/layer.test.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/integration/user/test/layer.test.js b/test/integration/user/test/layer.test.js index 8450872b6..4c431dbca 100644 --- a/test/integration/user/test/layer.test.js +++ b/test/integration/user/test/layer.test.js @@ -236,9 +236,11 @@ describe('Layer', () => { layer.addTo(map, 'watername_ocean'); // << adding before setting basemap map.setStyle(carto.basemaps.voyager); - map.on('load', () => { - expect(waitForMapToLoad).toHaveBeenCalled(); - done(); + layer.on('loaded', () => { + setTimeout(() => { + expect(waitForMapToLoad).toHaveBeenCalled(); + done(); + }, 0); }); }); }); From 44bbb4309fab703a297a055ff60012e28c2758e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 13:49:31 +0100 Subject: [PATCH 12/16] Just a test to check random test:user failures --- test/integration/user/test/layer.test.js | 52 ++++++++++++------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/test/integration/user/test/layer.test.js b/test/integration/user/test/layer.test.js index 4c431dbca..154efe0f2 100644 --- a/test/integration/user/test/layer.test.js +++ b/test/integration/user/test/layer.test.js @@ -1,5 +1,5 @@ import carto from '../../../../src/'; -import mapboxgl from 'mapbox-gl'; +// import mapboxgl from 'mapbox-gl'; import * as util from '../../util'; import { mat4 } from 'gl-matrix'; @@ -219,31 +219,31 @@ describe('Layer', () => { }); }); - describe('when map is not ready', () => { - it('should wait for it load', (done) => { - const name = 'map'; - div = util.createMapDivHolder(name); - map = new mapboxgl.Map({ - container: name, - center: [0, 0], - zoom: 0 - }); - - const source = new carto.source.GeoJSON(featureData); - const layer = new carto.Layer('layer', source, new carto.Viz()); - - const waitForMapToLoad = spyOn(layer, '_waitForMapToLoad').and.callThrough(); - layer.addTo(map, 'watername_ocean'); // << adding before setting basemap - map.setStyle(carto.basemaps.voyager); - - layer.on('loaded', () => { - setTimeout(() => { - expect(waitForMapToLoad).toHaveBeenCalled(); - done(); - }, 0); - }); - }); - }); + // describe('when map is not ready', () => { + // it('should wait for it load', (done) => { + // const name = 'map'; + // div = util.createMapDivHolder(name); + // map = new mapboxgl.Map({ + // container: name, + // center: [0, 0], + // zoom: 0 + // }); + + // const source = new carto.source.GeoJSON(featureData); + // const layer = new carto.Layer('layer', source, new carto.Viz()); + + // const waitForMapToLoad = spyOn(layer, '_waitForMapToLoad').and.callThrough(); + // layer.addTo(map, 'watername_ocean'); // << adding before setting basemap + // map.setStyle(carto.basemaps.voyager); + + // layer.on('loaded', () => { + // setTimeout(() => { + // expect(waitForMapToLoad).toHaveBeenCalled(); + // done(); + // }, 0); + // }); + // }); + // }); afterEach(() => { map.remove(); From a2b1514f0f415e4b1906ba0ebb31af3328556a07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 14:05:35 +0100 Subject: [PATCH 13/16] Split wait for map tests into a different file --- .../user/test/layer-waits-map.test.js | 45 +++ test/integration/user/test/layer.test.js | 341 ++++++++---------- 2 files changed, 201 insertions(+), 185 deletions(-) create mode 100644 test/integration/user/test/layer-waits-map.test.js diff --git a/test/integration/user/test/layer-waits-map.test.js b/test/integration/user/test/layer-waits-map.test.js new file mode 100644 index 000000000..493595c05 --- /dev/null +++ b/test/integration/user/test/layer-waits-map.test.js @@ -0,0 +1,45 @@ +import carto from '../../../../src/'; +import * as util from '../../util'; +import mapboxgl from 'mapbox-gl'; + +const featureData = { + type: 'Feature', + geometry: { + type: 'Point', + coordinates: [0, 0] + }, + properties: {} +}; + +describe('when map is not ready', () => { + let div, map; + + it('should wait for it load', (done) => { + const name = 'map'; + div = util.createMapDivHolder(name); + map = new mapboxgl.Map({ + container: name, + center: [0, 0], + zoom: 0 + }); + + const source = new carto.source.GeoJSON(featureData); + const layer = new carto.Layer('layer', source, new carto.Viz()); + + const waitForMapToLoad = spyOn(layer, '_waitForMapToLoad').and.callThrough(); + layer.addTo(map, 'watername_ocean'); // << adding before setting basemap + map.setStyle(carto.basemaps.voyager); + + layer.on('loaded', () => { + setTimeout(() => { + expect(waitForMapToLoad).toHaveBeenCalled(); + done(); + }, 0); + }); + }); + + afterEach(() => { + map.remove(); + document.body.removeChild(div); + }); +}); diff --git a/test/integration/user/test/layer.test.js b/test/integration/user/test/layer.test.js index 154efe0f2..faf950054 100644 --- a/test/integration/user/test/layer.test.js +++ b/test/integration/user/test/layer.test.js @@ -1,5 +1,4 @@ import carto from '../../../../src/'; -// import mapboxgl from 'mapbox-gl'; import * as util from '../../util'; import { mat4 } from 'gl-matrix'; @@ -15,235 +14,207 @@ const featureData = { describe('Layer', () => { let div, map, source, viz, layer; - describe('when a map is ready', () => { - beforeEach(() => { - const setup = util.createMap('map'); - div = setup.div; - map = setup.map; - - source = new carto.source.GeoJSON(featureData); - viz = new carto.Viz(` - @myColor: red - color: @myColor - `); - layer = new carto.Layer('layer', source, viz); - layer._paintLayer = () => { }; - layer.addTo(map); + beforeEach(() => { + const setup = util.createMap('map'); + div = setup.div; + map = setup.map; + + source = new carto.source.GeoJSON(featureData); + viz = new carto.Viz(` + @myColor: red + color: @myColor + `); + layer = new carto.Layer('layer', source, viz); + layer._paintLayer = () => { }; + layer.addTo(map); + }); + + describe('.on', () => { + it('should fire a "loaded" event when ready', (done) => { + layer.on('loaded', done); }); - describe('.on', () => { - it('should fire a "loaded" event when ready', (done) => { - layer.on('loaded', done); + describe('.updated', () => { + function mockPrerenderHelpers () { + // auxiliary mocks for prerender + spyOn(layer, '_getZoom').and.callFake(() => 0); + spyOn(layer, '_getViewport').and.callFake(() => { }); + spyOn(layer, '_needRefresh').and.callFake(() => Promise.resolve()); + } + + function resetMocks () { + layer._getZoom.calls.reset(); + layer._getViewport.calls.reset(); + layer._needRefresh.calls.reset(); + } + + it('should fire an "updated" event when ready', (done) => { + layer.on('updated', done); }); - describe('.updated', () => { - function mockPrerenderHelpers () { - // auxiliary mocks for prerender - spyOn(layer, '_getZoom').and.callFake(() => 0); - spyOn(layer, '_getViewport').and.callFake(() => { }); - spyOn(layer, '_needRefresh').and.callFake(() => Promise.resolve()); - } - - function resetMocks () { - layer._getZoom.calls.reset(); - layer._getViewport.calls.reset(); - layer._needRefresh.calls.reset(); - } - - it('should fire an "updated" event when ready', (done) => { - layer.on('updated', done); + it('should fire an "updated" event only once when ready', (done) => { + let update = jasmine.createSpy('update'); + layer.on('updated', update); + layer.on('loaded', () => { + setTimeout(() => { + expect(update).toHaveBeenCalled(); + done(); + }, 0); }); + }); + + it('should fire an "updated" event when the source is updated', (done) => { + mockPrerenderHelpers(); + layer.on('loaded', async () => { + const newSource = new carto.source.GeoJSON(featureData); + spyOn(newSource, 'requestData').and.callFake(() => true); - it('should fire an "updated" event only once when ready', (done) => { let update = jasmine.createSpy('update'); layer.on('updated', update); - layer.on('loaded', () => { - setTimeout(() => { - expect(update).toHaveBeenCalled(); - done(); - }, 0); - }); - }); - - it('should fire an "updated" event when the source is updated', (done) => { - mockPrerenderHelpers(); - layer.on('loaded', async () => { - const newSource = new carto.source.GeoJSON(featureData); - spyOn(newSource, 'requestData').and.callFake(() => true); - - let update = jasmine.createSpy('update'); - layer.on('updated', update); - await layer.update(newSource); - const currentMatrix = layer.renderer.matrix; - layer.prerender(null, currentMatrix); + await layer.update(newSource); + const currentMatrix = layer.renderer.matrix; + layer.prerender(null, currentMatrix); - expect(update).toHaveBeenCalledTimes(1); - resetMocks(); - done(); - }); + expect(update).toHaveBeenCalledTimes(1); + resetMocks(); + done(); }); + }); - it('should fire a new "updated" after moving the map (even if not requiring new dataframes)', (done) => { - mockPrerenderHelpers(); - - let numberOfUpdates = 0; - let update = () => { - numberOfUpdates++; + it('should fire a new "updated" after moving the map (even if not requiring new dataframes)', (done) => { + mockPrerenderHelpers(); - if (numberOfUpdates === 2) { - resetMocks(); - done(); // ...(b) we get a second update - return; - } + let numberOfUpdates = 0; + let update = () => { + numberOfUpdates++; - // // Emulate a user interaction, like dragging the map, using the underlying matrix - const matrix = layer.renderer.matrix; - let newMatrix = mat4.create(); - mat4.translate(newMatrix, matrix, [10, 0, 0]); // 10 units translation in x + if (numberOfUpdates === 2) { + resetMocks(); + done(); // ...(b) we get a second update + return; + } + + // // Emulate a user interaction, like dragging the map, using the underlying matrix + const matrix = layer.renderer.matrix; + let newMatrix = mat4.create(); + mat4.translate(newMatrix, matrix, [10, 0, 0]); // 10 units translation in x + + // (a) as it is a geojson, no dataframesHaveChange, but we need a new update anyway... + layer.prerender(null, newMatrix); + }; + layer.on('updated', update); + }); - // (a) as it is a geojson, no dataframesHaveChange, but we need a new update anyway... - layer.prerender(null, newMatrix); - }; + it('should fire an "updated" event when the viz is updated', (done) => { + mockPrerenderHelpers(); + layer.on('loaded', async () => { + let update = jasmine.createSpy('update'); layer.on('updated', update); - }); - - it('should fire an "updated" event when the viz is updated', (done) => { - mockPrerenderHelpers(); - layer.on('loaded', async () => { - let update = jasmine.createSpy('update'); - layer.on('updated', update); - await layer.update(source, new carto.Viz('color: blue')); - const currentMatrix = layer.renderer.matrix; - layer.prerender(null, currentMatrix); + await layer.update(source, new carto.Viz('color: blue')); + const currentMatrix = layer.renderer.matrix; + layer.prerender(null, currentMatrix); - expect(update).toHaveBeenCalled(); - resetMocks(); - done(); - }); + expect(update).toHaveBeenCalled(); + resetMocks(); + done(); }); + }); - it('should fire an "updated" event when the viz is animated', async (done) => { - let update = jasmine.createSpy('update'); - layer.on('updated', update); + it('should fire an "updated" event when the viz is animated', async (done) => { + let update = jasmine.createSpy('update'); + layer.on('updated', update); - await layer.update(source, new carto.Viz('width: now()')); + await layer.update(source, new carto.Viz('width: now()')); - layer.on('loaded', () => { - layer.render(); - expect(update).toHaveBeenCalled(); + layer.on('loaded', () => { + layer.render(); + expect(update).toHaveBeenCalled(); - layer.render(); - expect(update).toHaveBeenCalled(); - done(); - }); + layer.render(); + expect(update).toHaveBeenCalled(); + done(); }); }); + }); - describe('.hide', () => { - it('should hide a visible layer', (done) => { - layer.on('loaded', () => { - expect(layer.visible).toBeTruthy(); - layer.hide(); - expect(layer.visible).toBeFalsy(); - done(); - }); + describe('.hide', () => { + it('should hide a visible layer', (done) => { + layer.on('loaded', () => { + expect(layer.visible).toBeTruthy(); + layer.hide(); + expect(layer.visible).toBeFalsy(); + done(); }); + }); - it('should trigger an update event', (done) => { - layer.on('loaded', () => { - layer.on('updated', done); - layer.hide(); - }); + it('should trigger an update event', (done) => { + layer.on('loaded', () => { + layer.on('updated', done); + layer.hide(); }); + }); - it('should not request source data', (done) => { - layer.on('loaded', () => { - const requestDataSourceSpy = spyOn(layer._source, 'requestData'); - layer.hide(); - const currentMatrix = layer.renderer.matrix; - layer.prerender(null, currentMatrix); + it('should not request source data', (done) => { + layer.on('loaded', () => { + const requestDataSourceSpy = spyOn(layer._source, 'requestData'); + layer.hide(); + const currentMatrix = layer.renderer.matrix; + layer.prerender(null, currentMatrix); - expect(requestDataSourceSpy).not.toHaveBeenCalled(); - done(); - }); + expect(requestDataSourceSpy).not.toHaveBeenCalled(); + done(); }); }); + }); - describe('.show', () => { - beforeEach(() => { - layer.on('loaded', () => { - layer.hide(); - }); - }); - - it('should show a hidden layer', (done) => { - layer.on('loaded', () => { - expect(layer.visible).toBeFalsy(); - layer.show(); - expect(layer.visible).toBeTruthy(); - done(); - }); + describe('.show', () => { + beforeEach(() => { + layer.on('loaded', () => { + layer.hide(); }); + }); - it('should request source data', (done) => { - layer.on('loaded', () => { - const requestDataFn = layer._source.requestData.bind(layer._source); - let requestDataCalled = false; - layer._source.requestData = (...args) => { - requestDataCalled = true; - return requestDataFn(...args); - }; - // spyOn(layer._source, 'requestData'); - layer.hide(); - layer.show(); - layer.prerender(undefined, mat4.identity([])); - layer._matrix = mat4.identity([]); - layer._matrix[0] = 2; - layer.prerender(undefined, mat4.identity([])); - - expect(requestDataCalled).toBeTruthy(); - done(); - }); + it('should show a hidden layer', (done) => { + layer.on('loaded', () => { + expect(layer.visible).toBeFalsy(); + layer.show(); + expect(layer.visible).toBeTruthy(); + done(); }); }); - }); - describe('.blendToViz', () => { - it('should resolve the Promise with a valid viz', (done) => { + it('should request source data', (done) => { layer.on('loaded', () => { - layer.blendToViz(new carto.Viz('color: blue')).then(done); + const requestDataFn = layer._source.requestData.bind(layer._source); + let requestDataCalled = false; + layer._source.requestData = (...args) => { + requestDataCalled = true; + return requestDataFn(...args); + }; + // spyOn(layer._source, 'requestData'); + layer.hide(); + layer.show(); + layer.prerender(undefined, mat4.identity([])); + layer._matrix = mat4.identity([]); + layer._matrix[0] = 2; + layer.prerender(undefined, mat4.identity([])); + + expect(requestDataCalled).toBeTruthy(); + done(); }); }); }); }); - // describe('when map is not ready', () => { - // it('should wait for it load', (done) => { - // const name = 'map'; - // div = util.createMapDivHolder(name); - // map = new mapboxgl.Map({ - // container: name, - // center: [0, 0], - // zoom: 0 - // }); - - // const source = new carto.source.GeoJSON(featureData); - // const layer = new carto.Layer('layer', source, new carto.Viz()); - - // const waitForMapToLoad = spyOn(layer, '_waitForMapToLoad').and.callThrough(); - // layer.addTo(map, 'watername_ocean'); // << adding before setting basemap - // map.setStyle(carto.basemaps.voyager); - - // layer.on('loaded', () => { - // setTimeout(() => { - // expect(waitForMapToLoad).toHaveBeenCalled(); - // done(); - // }, 0); - // }); - // }); - // }); + describe('.blendToViz', () => { + it('should resolve the Promise with a valid viz', (done) => { + layer.on('loaded', () => { + layer.blendToViz(new carto.Viz('color: blue')).then(done); + }); + }); + }); afterEach(() => { map.remove(); From 52c17c4a1d26accd412a5049be9a1cb69dd69941 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 14:28:19 +0100 Subject: [PATCH 14/16] Make test:user:min happy again --- test/integration/user/test/layer-waits-map.test.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/test/integration/user/test/layer-waits-map.test.js b/test/integration/user/test/layer-waits-map.test.js index 493595c05..425a6e18c 100644 --- a/test/integration/user/test/layer-waits-map.test.js +++ b/test/integration/user/test/layer-waits-map.test.js @@ -14,7 +14,7 @@ const featureData = { describe('when map is not ready', () => { let div, map; - it('should wait for it load', (done) => { + it('Layer should wait for it load', (done) => { const name = 'map'; div = util.createMapDivHolder(name); map = new mapboxgl.Map({ @@ -30,12 +30,8 @@ describe('when map is not ready', () => { layer.addTo(map, 'watername_ocean'); // << adding before setting basemap map.setStyle(carto.basemaps.voyager); - layer.on('loaded', () => { - setTimeout(() => { - expect(waitForMapToLoad).toHaveBeenCalled(); - done(); - }, 0); - }); + expect(waitForMapToLoad).toHaveBeenCalled(); + done(); }); afterEach(() => { From ee667adc08d5d62cbebd7b0963a84090e2b1c458 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 16:40:18 +0100 Subject: [PATCH 15/16] Add fix description to changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cd180ce0..319165350 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Fix unwanted `Interactivity` events while using dragPan (and performance improvement) - Fix `linear` applied to cluster aggregations without explicit limits - Fix `MVT` category property returning a numeric value +- Fix `Layer.addTo` for MGL v0.52 ### Changed - Change examples to use Mapbox GL version 0.52 From 4bd4da7372c456397467f21eb52a0ccc65723764 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Velarde?= Date: Mon, 14 Jan 2019 16:42:43 +0100 Subject: [PATCH 16/16] Add missing description about helpers for browser support --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 319165350..74d27afe7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Add `enable`/`disable` methods and `isEnabled` property to `Interactivity` - Enable user-defined buckets in `viewportHistogram` - Add and improve examples (legends, widgets, labeling and basemaps) +- Add helpers to validate browser support (`isBrowserSupported` & `unsupportedBrowserReasons`) ### Fixed - Fix `linear` error when evaluating a date property in a feature