Use setData operation when diffing geojson sources #5332
Conversation
There was a problem hiding this comment.
Hi @zjernst thanks very much for this contribution!
A few comments inline, but it looks great overall.
src/source/source_cache.js
Outdated
| if (this._source && this._source.setData) { | ||
| this._source.setData(data); | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of adding this geojson-source-specific method to SourceCache, we can use sourceCache.getSource().setData(...) in the Style method handling this diff operation.
test/unit/style/style.test.js
Outdated
| style.on('style.load', () => { | ||
| const sourceCache = style.sourceCaches['source-id']; | ||
| t.spy(sourceCache, 'setData'); | ||
| style.setData('source-id', geoJSONSourceData); |
There was a problem hiding this comment.
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.)
src/style/style.js
Outdated
|
|
||
| if (this.sourceCaches[id] === undefined) { | ||
| throw new Error('There is no source with this ID'); | ||
| } |
There was a problem hiding this comment.
Since this is an internal invariant, please replace this check with just assert(this.sourceCaches[id] === undefined, 'There is no source with this ID')
src/style-spec/diff.js
Outdated
| @@ -44,6 +44,11 @@ const operations = { | |||
| removeSource: 'removeSource', | |||
|
|
|||
| /* | |||
| * { command: 'setData', args: ['sourceId', data] } | |||
| */ | |||
| setData: 'setData', | |||
There was a problem hiding this comment.
Let's call this setGeoJSONSourceData
|
@anandthakker Thanks for the review! Made changes to reflect your comments. Let me know how it looks |
src/source/source.js
Outdated
| @@ -57,6 +57,7 @@ export interface Source { | |||
|
|
|||
| +onAdd?: (map: Map) => void; | |||
| +onRemove?: (map: Map) => void; | |||
| +setData?: (data: GeoJSON | string) => Source; | |||
There was a problem hiding this comment.
Leftover -- this can be removed
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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').
|
@zjernst changes look good, thanks! One leftover: #5332 (review) @jfirebaugh @mourner FYI, this change makes |
|
@anandthakker Thanks for your help! Think it should be good now |
|
Thanks again @zjernst 🎉 ! |
#4914
When diffing the styles, if a geojson source is found with different data, perform a setData operation. Previously, the diff would cause a
removeSource/addSourcepair when a change was found, causing the source to flicker.I'm migrating my project from performing all diffs myself to keeping a style state tree and utilizing the smarter
setStylethat performs the necessary diffs, however the flickering was a noticeable downgrade to the experience.First contribution, so feel free to tear it apart/offer a better implementation!