-
-
Notifications
You must be signed in to change notification settings - Fork 4.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
Invalidate TileSource cache on property changes #8594
Changes from all commits
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 |
---|---|---|
|
@@ -20,7 +20,17 @@ export namespace TileSource { | |
initial_resolution: number | ||
} | ||
|
||
export interface Props extends Model.Props {} | ||
export interface Props extends Model.Props { | ||
url: p.Property<string> | ||
tile_size: p.Property<number> | ||
max_zoom: p.Property<number> | ||
min_zoom: p.Property<number> | ||
extra_url_vars: p.Property<{[key: string]: string}> | ||
attribution: p.Property<string> | ||
x_origin_offset: p.Property<number> | ||
y_origin_offset: p.Property<number> | ||
initial_resolution: p.Property<number> | ||
} | ||
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.
Yes and no. If you want automation, then yes. You may try to use 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. Thanks for the clarification, I think in this case it is needed because we want the model to be responsive to any change events, no matter how they were triggered. 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. Sure, the default is to use properties, especially as the part of the API. However, I just want to make sure that it's understood that the code above doesn't have any run time implications. Any interfaces or types (or anything else that is part of TypeScript and not JavaScript) are fully erased at compilation time. The only thing that matters at runtime is 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. Ah okay, I did misunderstand you initially then. Thanks for clarifying, the only thing I'm missing is why attributes and properties seem to be duplicating the same type information. Wouldn't it be sufficient to declare them once as either attributes or as properties? 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.
Yes (*), however, limitations of the language when we started the transition didn't allow for this. Currently I can use mapped and conditional types, to generate attributes from properties (this direction, because properties carry more information than attributes). I have a WIP PR that fixes that (after layout). However, (*) attributes and properties may, and in fact do in case of specs, carry different types, or even input (constructor) and output (model interface) types may differ. Altogether this can get pretty tricky, but with some type-level magic it can be handled without duplication. 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.
Just to add more color: the duplication is entirely because we developed a properties system in coffee script, if we had had the current version of typescript at the time we probably could have done else. Now we have the task of unifying our old sort-of-types with TypieScript's actual language capabilities, and that is an onerous task. 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. Thanks for the background, I really appreciate it. |
||
} | ||
|
||
export interface TileSource extends TileSource.Attrs {} | ||
|
@@ -60,6 +70,11 @@ export abstract class TileSource extends Model { | |
this._normalize_case() | ||
} | ||
|
||
connect_signals(): void { | ||
super.connect_signals() | ||
this.connect(this.change, () => this._clear_cache()) | ||
} | ||
|
||
string_lookup_replace(str: string, lookup: {[key: string]: string}): string { | ||
let result_str = str | ||
for (const key in lookup) { | ||
|
@@ -85,6 +100,10 @@ export abstract class TileSource extends Model { | |
this.url = url | ||
} | ||
|
||
protected _clear_cache(): void { | ||
this.tiles = {} | ||
} | ||
|
||
tile_xyz_to_key(x: number, y: number, z: number): string { | ||
return `${x}:${y}:${z}` | ||
} | ||
|
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.
So, what happens here if the tile source itself is swapped out?
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.
Ah, in that case I guess it should unsubscribe from this event, and subscribe to the new tile source. Is that handled on the
GlyphRenderer
when swapping out the Glyph? I can't spot where that's done.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.
Hrm I am not actually sure it is handled currently though I thought it was in the past. I would be OK with merging this and making a follow on issue to investigate more thorough signal disconnect/reconnect (in general, for glyphs and this and anything else)
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.
+1
Would be neat if the property could detect a model was being replaced and then dis- and reconnect any signals that were attached to the model being replaced, instead of having to handle this reconnecting logic all over the place. I don't know enough to determine whether that's feasible though.