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
6 changes: 6 additions & 0 deletions src/source/source_cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ class SourceCache extends Evented {
}
}

setData(data: GeoJSON | string) {
if (this._source && this._source.setData) {
this._source.setData(data);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding this geojson-source-specific method to SourceCache, we can use sourceCache.getSource().setData(...) in the Style method handling this diff operation.


/**
* Return true if no tile data is pending, tiles will not change unless
* an additional API call is received.
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: 'setData', args: ['sourceId', data] }
*/
setData: 'setData',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this 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 setData command to update
commands.push({ command: operations.setData, 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
24 changes: 23 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',
'setData'
// 'setGlyphs',
// 'setSprite',
]);
Expand Down Expand Up @@ -500,6 +501,27 @@ class Style extends Evented {
this._changed = true;
}

/**
* Set the data of a source, given its id. Only available for GeoJSON sources.
* @param {string} id id of the source
* @param {GeoJSON|string} data GeoJSON source
* @throws {Error} if no source is found with the given ID
*/

setData(id: string, data: GeoJSON | string) {
this._checkLoaded();

if (this.sourceCaches[id] === undefined) {
throw new Error('There is no source with this ID');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is an internal invariant, please replace this check with just assert(this.sourceCaches[id] === undefined, 'There is no source with this ID')



const sourceCache = this.sourceCaches[id];

sourceCache.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: 'setData', 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
59 changes: 59 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',
'setData',
'setLayerZoomRange',
'setLight'
].forEach((method) => t.stub(style, method).callsFake(() => t.fail(`${method} called`)));
Expand Down Expand Up @@ -644,6 +645,64 @@ test('Style#removeSource', (t) => {
t.end();
});

test('Style#setData', (t) => {
t.test('throw 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.setData('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.setData('source-id', geoJSONSourceData);
}, /There is no source with this ID/);
t.end();
});
});

t.test('sets data of source', (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] }
}
]
};
style.on('style.load', () => {
const sourceCache = style.sourceCaches['source-id'];
t.spy(sourceCache, 'setData');
style.setData('source-id', geoJSONSourceData);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of testing style.setData(), could you please rework this test to use style.setState()? The latter is part of the interface of Style from the perspective of the rest of the codebase, whereas the former is essentially private. (Admittedly, this isn't very clearly documented or enforced at the moment.)

t.ok(sourceCache.setData.calledWith(geoJSONSourceData));
t.end();
});
});

t.end();
});

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