The beginning of a series of refactoring PRs to get rid of the legacy callback-event-based communication model in favor of a Promise-based approach.
What's done by this PR:
-
First and foremost: refactored the src/util/ajax.ts file
- Reconsidered all its functions. The ones that were not used (like
postData) are removed
- All the separate small utility functions which were used solely by the same
ajax.ts file are incorporated into the bigger functions which are used by the code outside of the file. E.g. isSameOrigin was used only once by getVideo, so now it's a part of getVideo. Some other functions splitted into multiple sub-functions are combined into a single one to avoid confusion about what's exported why and what's used where. E.g. arrayBufferToCanvasImageSource now combines in itself 2 small sub-functions that were previously 2 separate ones.
- All the methods are exported now to be able to test them, but the ones not used in runtime are marked as
@private. They are also in the bottom of the file.
- All the functions are JSDoc-commented.
- Important. Public functions like
makeRequest now don't accept a callback function anymore. Instead, they return an object consisting of 2 fields: response and cancel. response is a promise that resolves with 3 fields: data (holds the actual response), cacheControl and expires. The last two hold the values of respective response's HTTP headers. There are places in consumer-code that rely on these fields. Though still a point to consider: do we really need manual cache control?
- Everything (almost) is covered with unit tests. Coverage % is now >99. The only not-enough-tested function is
getReferer, because I don't know how to test the case when the current url is a data blob.
getReferrer is renamed to getReferer (and obviously for all usages as well) to better reflect that it's about the referer HTTP header. Yes, it's spelled with one "r".- Both Fetch API and XHR API are left intact. XHR is not removed due to https://github.com/maplibre/maplibre-gl-js/discussions/2004
AJAXError is completely removed from the project. There's no need in it since all the fetching-related errors can be represented using existing classes. Now it's done using the most generic Error, but that's a point to reconsider- Important.
tile_request_cache dep is removed. See below for clarifications
- TEMP
getImage doesn't support max parallel requests. But did it really?.. Needs to be resolved before merging.
-
package.json
- introduces a new dependency for mocking the Fetch API
- Removed the documentation lint script because it fails and doesn't allow to run unit tests. Not sure, why. Need help with this as this needs to be resolved before merging the PR.
-
index.ts
AJAXError is removed. See aboveclearStorage public method is removed due to manual tile caching control removal. See belowaddProtocol: updated documentation (see the relevant discussion in Slack) and API (to correspond to the updated ajax model).
-
map.ts - _setCacheLimits is removed due to removal of tile_request_cache. See clarifications below
-
Sources, utils, UI, etc. (src/...) - See below
Removed tile_request_cache
See https://github.com/maplibre/maplibre-gl-js/discussions/2010
Seems like this file for manual tile cache control was not used at all. The research shows that it used to exist only for solving some very old target env.-specific bugs (namely safari) that have already been fixed long time ago.
From now on all the cache-manipulation should be performed by the browser based on request and response headers. We should not do it manually.
Sources, utils, UI, etc.
All the other modified files are modified just slightly and just to support the new Promise-based model. There's a thing however.
Instead of getting completely rid of all the callbacks, I only got rid of them in ajax.ts file. Therefore all the consumer code must still communicate with all the other code using callbacks and events. Oherwise it would be too much of a change for a single PR. So, for example. If loadTile method in geojson_source uses getJSON to load some json and getJSON now returnes an object with a promise, we can not forward it to the next consumer as is.
What we do instead, is we await the promise (with then not to make the function async - for compatibility with existting code) and only after then we invoke the consumer's callback. Basically, just an adapter pattern (but not splitted into a separate function to avoid overcomplicating things):
Before:
function loadVectorTile(params: WorkerTileParameters, callback: LoadVectorDataCallback) {
const request = getArrayBuffer(params.request, (err?: Error | null, data?: ArrayBuffer | null, cacheControl?: string | null, expires?: string | null) => {
if (err) {
callback(err);
} else if (data) {
callback(null, {
vectorTile: new vt.VectorTile(new Protobuf(data)),
rawData: data,
cacheControl,
expires
});
}
});
return () => {
request.cancel();
callback();
};
}
After:
function loadVectorTile(params: WorkerTileParameters, callback: LoadVectorDataCallback) {
const request = getArrayBuffer(params.request);
request.response.then((response) => {
callback(null, {
vectorTile: new vt.VectorTile(new Protobuf(response.data)),
rawData: response.data,
cacheControl: response.cacheControl,
expires: response.expires
});
}).catch(err => {
if (err.message === 'aborted') return; // *
callback(err);
});
return () => {
request.cancel();
callback();
};
}
Once again, the idea is to further refact the loadVectorTile function to become an async function and remove its callback.
The line above marked with * is a one to pay attention to.
Since now, if a request was canceled, the response promise of it throws an Error("aborted"). This is done for 2 reasons:
- It's now impossible to just skip callback invokation as it used to be (because there's no callback anymore)
- IMHO, it's more logical then resolving the promise with some synthetic value (like
undefined e.g.). At least, it's safer. At least in theory.
Here's a rough list of other files which could be considered ajax.ts's deps:
getJSON()
-----
geojson_worker_source.ts:227
load_tilejson.ts:37
load_sprite:30
style.ts:278
map.ts:1523
==========
getArrayBuffer()
-----
ajax.ts:385
rtl_text_plugin.ts:83
load_glyph_range.ts:19
vector_tile_worker_source:43
==========
getImage()
-----
image_source.ts:114
raster_dem_tile_source.ts:41
raster_tile_source.ts:106
load_sprite.ts:46
map.ts:1953
==========
getVideo()
-----
video_source.ts:72
Any further refactoring should start with any of these files ideally.
Tests (other than ajax.test.ts)
There's no huge changes to the tests, but still. Anytime you change even a single character in tests, the changes are considered breaking.
Because of geting rid of type and const for resource type and substtituting them with and enum MapLibreResourceType, for some tests it was necessary to update the values accordingly. E.g. style.test.ts. In some tests that was the only change. So they could be considered safe.
For some other tests (like the same style.tes.ts) some tests where removed (in this case due to changes to loadSprite (mentioned below)).
Another interesting tests-related thing to mention. Take a look:
I don't know why it used to work previously. Is that the case of event loop? When Promises take longer than callbacks. So the changes now don't apply immediately, but rather after the event was thrown. Shouldn't that have been the case with callbacks as well though? Not sure...
loadSprite
Removed request manager passed as a parameter. Don't see any need it passing it around when it can be created on demand.
Moreover, don't see any real reason to have it at all. The name is very misleading. It doesn't manage anything, but rather it's just a set of incapsulated methods (which could be separated into a module instead).
Summary.
Some changes (like _setCacheLimits and addProtocol) are breaking. Especially addProtocol, because it's public. _setCacheLimits is not, AFAICS. Even though it's exported, it's not documented anywhere. So, this is a v3.0.0 candidate, not v2.x.x.
The only final version (after all the code comments are resolved!!!) is ajax.ts and its unit tests. All the other modified files should be working as expected, but need further refactoring.
Tell me please if I forgot something. Will also try to add some code comments through review now.
1.6k Jan 5, 2023
19 Dec 29, 2022
1.8k Dec 20, 2022
117 Jan 1, 2023
392 Dec 24, 2022
10 Nov 18, 2022
2k Jan 9, 2023
2 Mar 13, 2022
222 Nov 8, 2022
44 Feb 17, 2022
4.5k Dec 24, 2022
314 Nov 13, 2022
1.6k Jan 2, 2023
3.3k Jan 4, 2023
1k Dec 23, 2022
4.4k Jan 5, 2023
0 Dec 16, 2021
54 Sep 7, 2022
20 Mar 9, 2022