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

Replace getJson call with async code and remove more callbacks #3273

Merged
merged 12 commits into from
Oct 27, 2023
12 changes: 6 additions & 6 deletions .github/workflows/test-all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ jobs:
- uses: actions/checkout@v4
- uses: actions/setup-node@v3
with:
node-version: lts/*
node-version-file: '.nvmrc'
- run: npm ci
- run: npm run lint
if: success() || failure()
Expand All @@ -43,7 +43,7 @@ jobs:
- uses: actions/checkout@v4
- uses: actions/setup-node@v3
with:
node-version: lts/*
node-version-file: '.nvmrc'
- name: Start display server
if: runner.os == 'Linux'
run: nohup Xvfb &
Expand Down Expand Up @@ -73,7 +73,7 @@ jobs:
- uses: actions/checkout@v4
- uses: actions/setup-node@v3
with:
node-version: lts/*
node-version-file: '.nvmrc'
- run: npm ci
- run: npm run build-dist
- uses: actions/upload-artifact@v3
Expand All @@ -99,7 +99,7 @@ jobs:
- uses: actions/checkout@v4
- uses: actions/setup-node@v3
with:
node-version: lts/*
node-version-file: '.nvmrc'
- name: install CJK fonts for local ideographs render tests
run: sudo apt install fonts-noto-cjk
- run: npm ci
Expand Down Expand Up @@ -134,7 +134,7 @@ jobs:
- uses: actions/checkout@v4
- uses: actions/setup-node@v3
with:
node-version: lts/*
node-version-file: '.nvmrc'
- run: npm ci
- name: Start display server
if: runner.os == 'Linux'
Expand Down Expand Up @@ -162,7 +162,7 @@ jobs:
- uses: actions/checkout@v4
- uses: actions/setup-node@v3
with:
node-version: lts/*
node-version-file: '.nvmrc'
- run: npm ci
- run: npm run build-dist
- run: npm run test-build
Expand Down
20 changes: 13 additions & 7 deletions src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,8 @@ describe('maplibre', () => {
callback(null, {'foo': 'bar'});
return {cancel: () => {}};
});
getJSON({url: 'custom://test/url/json'}, (error, data) => {
expect(error).toBeFalsy();
expect(data).toEqual({foo: 'bar'});
getJSON({url: 'custom://test/url/json'}, new AbortController()).then((response) => {
expect(response.data).toEqual({foo: 'bar'});
expect(protocolCallbackCalled).toBeTruthy();
done();
});
Expand Down Expand Up @@ -101,20 +100,27 @@ describe('maplibre', () => {
return {cancel: () => { }};
});

getJSON({url: 'custom://test/url/json'}, (error) => {
getJSON({url: 'custom://test/url/json'}, new AbortController()).catch((error) => {
expect(error).toBeTruthy();
});
});

test('#addProtocol - Cancel request', () => {
test('#addProtocol - Cancel request', async () => {
let cancelCalled = false;
maplibre.addProtocol('custom', () => {
return {cancel: () => {
cancelCalled = true;
}};
});
const request = getJSON({url: 'custom://test/url/json'}, () => { });
request.cancel();
const abortController = new AbortController();
const promise = getJSON({url: 'custom://test/url/json'}, abortController);
abortController.abort();
try {
await promise;
} catch (err) {
expect(err.message).toBe('AbortError');
}

expect(cancelCalled).toBeTruthy();
});

Expand Down
84 changes: 38 additions & 46 deletions src/source/geojson_worker_source.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,7 @@ describe('resourceTiming', () => {
window.performance.getEntriesByName = jest.fn().mockReturnValue([exampleResourceTiming]);

const layerIndex = new StyleLayerIndex(layers);
const source = new GeoJSONWorkerSource(actor, layerIndex, [], (params, callback) => {
callback(null, geoJson);
return {cancel: () => {}};
});
const source = new GeoJSONWorkerSource(actor, layerIndex, [], () => Promise.resolve(geoJson));

source.loadData({source: 'testSource', request: {url: 'http://localhost/nonexistent', collectResourceTiming: true}} as LoadGeoJSONParameters)
.then((result) => {
Expand Down Expand Up @@ -149,10 +146,7 @@ describe('resourceTiming', () => {
jest.spyOn(perf, 'clearMeasures').mockImplementation(() => { return null; });

const layerIndex = new StyleLayerIndex(layers);
const source = new GeoJSONWorkerSource(actor, layerIndex, [], (params, callback) => {
callback(null, geoJson);
return {cancel: () => {}};
});
const source = new GeoJSONWorkerSource(actor, layerIndex, [], () => Promise.resolve(geoJson));

source.loadData({source: 'testSource', request: {url: 'http://localhost/nonexistent', collectResourceTiming: true}} as LoadGeoJSONParameters)
.then((result) => {
Expand Down Expand Up @@ -219,53 +213,51 @@ describe('loadData', () => {

const layerIndex = new StyleLayerIndex(layers);
function createWorker() {
const worker = new GeoJSONWorkerSource(actor, layerIndex, []);

// Making the call to loadGeoJSON asynchronous
// allows these tests to mimic a message queue building up
// (regardless of timing)
const originalLoadGeoJSON = worker.loadGeoJSON;
worker.loadGeoJSON = function(params, callback) {
const timeout = setTimeout(() => {
originalLoadGeoJSON(params, callback);
}, 0);

return {cancel: () => clearTimeout(timeout)};
};
return worker;
return new GeoJSONWorkerSource(actor, layerIndex, []);
}

test('abandons previous callbacks', done => {
test('abandons previous callbacks', async () => {
const worker = createWorker();
let firstCallbackHasRun = false;

worker.loadData({source: 'source1', data: JSON.stringify(geoJson)} as LoadGeoJSONParameters).then((result) => {
expect(result && result.abandoned).toBeTruthy();
firstCallbackHasRun = true;
}).catch(() => expect(false).toBeTruthy());
server.respondWith(request => {
request.respond(200, {'Content-Type': 'application/json'}, JSON.stringify(geoJson));
});

worker.loadData({source: 'source1', data: JSON.stringify(geoJson)} as LoadGeoJSONParameters).then((result) => {
expect(result && result.abandoned).toBeFalsy();
expect(firstCallbackHasRun).toBeTruthy();
done();
}).catch(() => expect(false).toBeTruthy());
const p1 = worker.loadData({source: 'source1', request: {url: ''}} as LoadGeoJSONParameters);
await new Promise((resolve) => (setTimeout(resolve, 0)));

const p2 = worker.loadData({source: 'source1', request: {url: ''}} as LoadGeoJSONParameters);

await new Promise((resolve) => (setTimeout(resolve, 0)));

server.respond();

const firstCallResult = await p1;
expect(firstCallResult && firstCallResult.abandoned).toBeTruthy();
const result = await p2;
expect(result && result.abandoned).toBeFalsy();
});

test('removeSource aborts callbacks', done => {
test('removeSource aborts callbacks', async () => {
const worker = createWorker();
let loadDataCallbackHasRun = false;
worker.loadData({source: 'source1', data: JSON.stringify(geoJson)} as LoadGeoJSONParameters).then((result) => {
expect(result && result.abandoned).toBeTruthy();
loadDataCallbackHasRun = true;
}).catch(() => expect(false).toBeTruthy());

worker.removeSource({source: 'source1', type: 'type'}).then(() => {
// Allow promsie to resolve
setTimeout(() => {
expect(loadDataCallbackHasRun).toBeTruthy();
done();
}, 0);
}).catch(() => expect(false).toBeTruthy());
server.respondWith(request => {
request.respond(200, {'Content-Type': 'application/json'}, JSON.stringify(geoJson));
});

const loadPromise = worker.loadData({source: 'source1', request: {url: ''}} as LoadGeoJSONParameters);

await new Promise((resolve) => (setTimeout(resolve, 0)));

const removePromise = worker.removeSource({source: 'source1', type: 'type'});

await new Promise((resolve) => (setTimeout(resolve, 0)));

server.respond();

const result = await loadPromise;
expect(result && result.abandoned).toBeTruthy();
await removePromise;
});

test('loadData with geojson creates an non-updateable source', done => {
Expand Down
Loading