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

Use setData operation when diffing geojson sources #5332

Merged
merged 9 commits into from
Sep 26, 2017
1 change: 1 addition & 0 deletions src/source/source.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export interface Source {

+onAdd?: (map: Map) => void;
+onRemove?: (map: Map) => void;
+setData?: (data: GeoJSON | string) => Source;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover -- this can be removed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but I believe I still need to add this to the Source interface so setData can be called in Style#setGeoJSONSourceData. I'm getting a flow error of property setData of unknown type when I remove this line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. setData is defined on GeoJSONSource. So, instead of adding it to the general Source interface, you can do the following in setGeoJSONSourceData:

const geojsonSource: GeoJSONSource = (this.sourceCaches[id].getSource(): any);
geojsonSource.setData()

Note that the use of : any essentially disables Flow's checking, so it should be used with caution. As a general practice, I try to include some sort of runtime check to accompany any use of : any -- e.g., assert(geojsonSource.type === 'geojson').

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Gotcha -- that makes sense


loadTile(tile: Tile, callback: Callback<void>): void;
+hasTile?: (coord: TileCoord) => boolean;
Expand Down
18 changes: 14 additions & 4 deletions src/style-spec/diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ const operations = {
*/
removeSource: 'removeSource',

/*
* { command: 'setGeoJSONSourceData', args: ['sourceId', data] }
*/
setGeoJSONSourceData: 'setGeoJSONSourceData',

/*
* { command: 'setLayerZoomRange', args: ['layerId', 0, 22] }
*/
Expand Down Expand Up @@ -117,10 +122,15 @@ function diffSources(before, after, commands, sourcesRemoved) {
if (!before.hasOwnProperty(sourceId)) {
commands.push({ command: operations.addSource, args: [sourceId, after[sourceId]] });
} else if (!isEqual(before[sourceId], after[sourceId])) {
// no update command, must remove then add
commands.push({ command: operations.removeSource, args: [sourceId] });
commands.push({ command: operations.addSource, args: [sourceId, after[sourceId]] });
sourcesRemoved[sourceId] = true;
if (before[sourceId].type === 'geojson' && after[sourceId].type === 'geojson') {
// geojson sources use setGeoJSONSourceData command to update
commands.push({ command: operations.setGeoJSONSourceData, args: [sourceId, after[sourceId].data] });
} else {
// no update command, must remove then add
commands.push({ command: operations.removeSource, args: [sourceId] });
commands.push({ command: operations.addSource, args: [sourceId, after[sourceId]] });
sourcesRemoved[sourceId] = true;
}
}
}
}
Expand Down
20 changes: 19 additions & 1 deletion src/style/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ const supportedDiffOperations = util.pick(diff.operations, [
'removeSource',
'setLayerZoomRange',
'setLight',
'setTransition'
'setTransition',
'setGeoJSONSourceData'
// 'setGlyphs',
// 'setSprite',
]);
Expand Down Expand Up @@ -500,6 +501,23 @@ class Style extends Evented {
this._changed = true;
}

/**
* Set the data of a GeoJSON source, given its id.
* @param {string} id id of the source
* @param {GeoJSON|string} data GeoJSON source
*/
setGeoJSONSourceData(id: string, data: GeoJSON | string) {
this._checkLoaded();

assert(this.sourceCaches[id] !== undefined, 'There is no source with this ID');
const source = this.sourceCaches[id].getSource();

if (source.setData) {
source.setData(data);
this._changed = true;
}
}

/**
* Get a source by id.
* @param {string} id id of the desired source
Expand Down
30 changes: 30 additions & 0 deletions test/unit/style-spec/diff.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,36 @@ t('diff', (t) => {
{ command: 'addSource', args: ['foo', 1] }
], 'add a source');

t.deepEqual(diffStyles({
sources: {
foo: {
type: 'geojson',
data: { type: 'FeatureCollection', features: [] }
}
}
}, {
sources: {
foo: {
type: 'geojson',
data: {
type: 'FeatureCollection',
features: [{
type: 'Feature',
geometry: { type: 'Point', coordinates: [10, 20] }
}]
}
}
}
}), [
{ command: 'setGeoJSONSourceData', args: ['foo', {
type: 'FeatureCollection',
features: [{
type: 'Feature',
geometry: { type: 'Point', coordinates: [10, 20] }
}]
}]}
], 'update a geojson source');

t.deepEqual(diffStyles({}, {
metadata: { 'mapbox:author': 'nobody' }
}), [], 'ignore style metadata');
Expand Down
80 changes: 80 additions & 0 deletions test/unit/style/style.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,7 @@ test('Style#setState', (t) => {
'setFilter',
'addSource',
'removeSource',
'setGeoJSONSourceData',
'setLayerZoomRange',
'setLight'
].forEach((method) => t.stub(style, method).callsFake(() => t.fail(`${method} called`)));
Expand Down Expand Up @@ -456,6 +457,49 @@ test('Style#setState', (t) => {
});
});

t.test('sets GeoJSON source data if different', (t) => {
const initialState = createStyleJSON({
"sources": { "source-id": createGeoJSONSource() }
});

const geoJSONSourceData = {
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"geometry": {
"type": "Point",
"coordinates": [125.6, 10.1]
}
}
]
};

const nextState = createStyleJSON({
"sources": {
"source-id": {
"type": "geojson",
"data": geoJSONSourceData
}
}
});

const style = new Style(initialState);

style.on('style.load', () => {
const geoJSONSource = style.sourceCaches['source-id'].getSource();
t.spy(style, 'setGeoJSONSourceData');
t.spy(geoJSONSource, 'setData');
const didChange = style.setState(nextState);

t.ok(style.setGeoJSONSourceData.calledWith('source-id', geoJSONSourceData));
t.ok(geoJSONSource.setData.calledWith(geoJSONSourceData));
t.ok(didChange);
t.same(style.stylesheet, nextState);
t.end();
});
});

t.end();
});

Expand Down Expand Up @@ -644,6 +688,42 @@ test('Style#removeSource', (t) => {
t.end();
});

test('Style#setData', (t) => {
t.test('throws before loaded', (t) => {
const style = new Style(createStyleJSON({
"sources": { "source-id": createGeoJSONSource() }
}), new StubMap());
const geoJSONSourceData = {
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"geometry": { "type": "Point", "coordinates": [125.6, 10.1] }
}
]
};
t.throws(() => {
style.setGeoJSONSourceData('source-id', geoJSONSourceData);
}, Error, /load/i);
style.on('style.load', () => {
t.end();
});
});

t.test('throws on non-existence', (t) => {
const style = new Style(createStyleJSON(), new StubMap()),
geoJSONSourceData = { type: "FeatureCollection", "features": [] };
style.on('style.load', () => {
t.throws(() => {
style.setGeoJSONSourceData('source-id', geoJSONSourceData);
}, Error, /There is no source with this ID/);
t.end();
});
});

t.end();
});

test('Style#addLayer', (t) => {
t.test('throw before loaded', (t) => {
const style = new Style(createStyleJSON(), new StubMap()),
Expand Down