-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Maps] Fix tileload error messaging / Fix heatmap error message #30327
Changes from 3 commits
5deee72
1128845
86403e5
d431d2d
20693bb
b8ab7ed
88f4b6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,8 +10,6 @@ import React from 'react'; | |
import { EuiIcon } from '@elastic/eui'; | ||
import { TileStyle } from '../layers/styles/tile_style'; | ||
|
||
const TMS_LOAD_TIMEOUT = 32000; | ||
|
||
export class TileLayer extends AbstractLayer { | ||
|
||
static type = "TILE"; | ||
|
@@ -32,38 +30,6 @@ export class TileLayer extends AbstractLayer { | |
return tileLayerDescriptor; | ||
} | ||
|
||
_tileLoadErrorTracker(map, url) { | ||
let tileLoad; | ||
map.on('dataloading', ({ tile }) => { | ||
if (tile && tile.request) { | ||
// If at least one tile loads, endpoint/resource is valid | ||
tile.request.onloadend = ({ loaded }) => { | ||
if (loaded) { | ||
tileLoad = true; | ||
} | ||
}; | ||
} | ||
}); | ||
|
||
return new Promise((resolve, reject) => { | ||
let tileLoadTimer = null; | ||
|
||
const clearChecks = () => { | ||
clearTimeout(tileLoadTimer); | ||
map.off('dataloading'); | ||
}; | ||
|
||
tileLoadTimer = setTimeout(() => { | ||
if (!tileLoad) { | ||
reject(new Error(`Tiles from "${url}" could not be loaded`)); | ||
} else { | ||
resolve(); | ||
} | ||
clearChecks(); | ||
}, TMS_LOAD_TIMEOUT); | ||
}); | ||
} | ||
|
||
async syncData({ startLoading, stopLoading, onLoadError, dataFilters }) { | ||
if (!this.isVisible() || !this.showAtZoomLevel(dataFilters.zoom)) { | ||
return; | ||
|
@@ -79,41 +45,37 @@ export class TileLayer extends AbstractLayer { | |
} | ||
} | ||
|
||
async syncLayerWithMB(mbMap) { | ||
syncLayerWithMB(mbMap) { | ||
|
||
const source = mbMap.getSource(this.getId()); | ||
const mbLayerId = this.getId() + '_raster'; | ||
|
||
if (source) { | ||
// If source exists, just sync style | ||
this._setTileLayerProperties(mbMap, mbLayerId); | ||
return; | ||
} | ||
if (!source) { | ||
const sourceDataRquest = this.getSourceDataRequest(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: sourceDataRquest -> sourceDataRequest |
||
const url = sourceDataRquest.getData(); | ||
if (!url) { | ||
return; | ||
} | ||
|
||
const sourceDataRquest = this.getSourceDataRequest(); | ||
const url = sourceDataRquest.getData(); | ||
if (!url) { | ||
const sourceId = this.getId(); | ||
mbMap.addSource(sourceId, { | ||
type: 'raster', | ||
tiles: [url], | ||
tileSize: 256, | ||
scheme: 'xyz', | ||
}); | ||
|
||
mbMap.addLayer({ | ||
id: mbLayerId, | ||
type: 'raster', | ||
source: sourceId, | ||
minzoom: this._descriptor.minZoom, | ||
maxzoom: this._descriptor.maxZoom, | ||
}); | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. Good call returning before a needless call to set layer props There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops, that's actually a bug... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol. I was actually just staring at it coming to the same conclusion. we would be doing an extra call to set zoom but alpha isn't covered. there might be a better way to dice those up but a redundant zoom set isn't the end of the world |
||
} | ||
|
||
const sourceId = this.getId(); | ||
mbMap.addSource(sourceId, { | ||
type: 'raster', | ||
tiles: [url], | ||
tileSize: 256, | ||
scheme: 'xyz', | ||
}); | ||
|
||
mbMap.addLayer({ | ||
id: mbLayerId, | ||
type: 'raster', | ||
source: sourceId, | ||
minzoom: this._descriptor.minZoom, | ||
maxzoom: this._descriptor.maxZoom, | ||
}); | ||
this._setTileLayerProperties(mbMap, mbLayerId); | ||
|
||
await this._tileLoadErrorTracker(mbMap, url); | ||
} | ||
|
||
_setTileLayerProperties(mbMap, mbLayerId) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems error prone to have
this._dataRequests
with a single underscore andthis._descriptor.__dataRequests
with a double underscore. Any ideas on how to avoid?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah .... it looks funny.
We could just revert to single underscore, and ditch the double underscore.
On the other hand, it's a pretty common convention to prepend
_
to variables of private state or internal state. But I feel like store-state is something different altogether (just gut feeling..)Maybe we could just leave as-is for now (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, leaving as is for now sounds ok.