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

Move addSourceType to global object #3420

Merged
merged 5 commits into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
## main

### ✨ Features and improvements

- ⚠️ Moved the `addSrouceType` to be a part of the global maplibregl object instead of being per map object ([#3420](https://github.com/maplibre/maplibre-gl-js/pull/3420))
- _...Add new stuff here..._

### 🐞 Bug fixes
Expand Down
11 changes: 10 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {RasterDEMTileSource} from './source/raster_dem_tile_source';
import {RasterTileSource} from './source/raster_tile_source';
import {VectorTileSource} from './source/vector_tile_source';
import {VideoSource} from './source/video_source';

import {addSourceType, type SourceClass} from './source/source';
const version = packageJSON.version;

export type * from '@maplibre/maplibre-gl-style-spec';
Expand Down Expand Up @@ -231,6 +231,15 @@ class MapLibreGL {
static removeProtocol(customProtocol: string) {
delete config.REGISTERED_PROTOCOLS[customProtocol];
}

Copy link
Collaborator

@neodescis neodescis Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we expose getSourceType here as well? Seems like we should.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds right, but what would you use if for?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that comes to mind is allowing the source type to be changed at runtime. Allowing retrieval of the type would allow for that, without having to keep track of it outside of MapLibre. Of course, I realize changing some of the things about a source type at runtime would be dangerous...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anything, remove would be the opposite of add. but I'm not sure how to "remove" code that was added to the worker.
And yes, changing a source is super risky...
I'm not sure either way... I think this API isn't really in use as it is so complicated to get it right...
In general you can mess with the prototype of the source object without the need for this method basically.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, understood. I'm also perfectly fine if we leave it as-is for now. Just food for thought.

/**
* Adds a [custom source type](#Custom Sources), making it available for use with
* {@link Map#addSource}.
* @param name - The name of the source type; source definition objects use this name in the `{type: ...}` field.
* @param SourceType - A {@link SourceClass} - which is a constructor for the {@link Source} interface.
* @returns a promise that is resolved when the source type is ready or with an error argument if there is an error.
*/
static addSourceType = (name: string, sourceType: SourceClass) => addSourceType(name, sourceType);
}

//This gets automatically stripped out in production builds.
Expand Down
51 changes: 51 additions & 0 deletions src/source/source.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import {Dispatcher} from '../util/dispatcher';
import {SourceClass, addSourceType, create} from './source';

describe('addSourceType', () => {
test('adds factory function without a worker url does not dispatch to worker', async () => {
const sourceType = jest.fn().mockImplementation(function (id) { this.id = id; }) as SourceClass;

// expect no call to load worker source
const spy = jest.spyOn(Dispatcher.prototype, 'broadcast');

await addSourceType('foo', sourceType);
expect(spy).not.toHaveBeenCalled();

create('id', {type: 'foo'} as any, null, null);
expect(sourceType).toHaveBeenCalled();
});

test('create a custom source without an id throws', async () => {
const sourceType = jest.fn() as SourceClass;

// expect no call to load worker source
const spy = jest.spyOn(Dispatcher.prototype, 'broadcast');

await addSourceType('foo2', sourceType);
expect(spy).not.toHaveBeenCalled();

expect(() => create('id', {type: 'foo2'} as any, null, null)).toThrow();
expect(sourceType).toHaveBeenCalled();
});

test('triggers workers to load worker source code', async () => {
const sourceType = function () {} as any as SourceClass;
sourceType.workerSourceURL = 'worker-source.js' as any as URL;

const spy = jest.spyOn(Dispatcher.prototype, 'broadcast');

await addSourceType('bar', sourceType);
expect(spy).toHaveBeenCalledWith('loadWorkerSource', 'worker-source.js');
});

test('refuses to add new type over existing name', async () => {
const sourceType = function () {} as any as SourceClass;
await expect(addSourceType('canvas', sourceType)).rejects.toThrow();
await expect(addSourceType('geojson', sourceType)).rejects.toThrow();
await expect(addSourceType('image', sourceType)).rejects.toThrow();
await expect(addSourceType('raster', sourceType)).rejects.toThrow();
await expect(addSourceType('raster-dem', sourceType)).rejects.toThrow();
await expect(addSourceType('vector', sourceType)).rejects.toThrow();
await expect(addSourceType('video', sourceType)).rejects.toThrow();
});
});
22 changes: 19 additions & 3 deletions src/source/source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ import {GeoJSONSource} from '../source/geojson_source';
import {VideoSource} from '../source/video_source';
import {ImageSource} from '../source/image_source';
import {CanvasSource} from '../source/canvas_source';
import {Dispatcher} from '../util/dispatcher';
import {getGlobalWorkerPool} from '../util/global_worker_pool';

import type {SourceSpecification} from '@maplibre/maplibre-gl-style-spec';
import type {Dispatcher} from '../util/dispatcher';
import type {Event, Evented} from '../util/evented';
import type {Map} from '../ui/map';
import type {Tile} from './tile';
Expand Down Expand Up @@ -159,7 +160,7 @@ export const create = (id: string, specification: SourceSpecification | CanvasSo
return source;
};

export const getSourceType = (name: string): SourceClass => {
const getSourceType = (name: string): SourceClass => {
switch (name) {
case 'geojson':
return GeoJSONSource;
Expand All @@ -179,6 +180,21 @@ export const getSourceType = (name: string): SourceClass => {
return registeredSources[name];
};

export const setSourceType = (name: string, type: SourceClass) => {
const setSourceType = (name: string, type: SourceClass) => {
registeredSources[name] = type;
};

export const addSourceType = async (name: string, SourceType: SourceClass): Promise<void> => {
if (getSourceType(name)) {
throw new Error(`A source type called "${name}" already exists.`);
}

setSourceType(name, SourceType);

if (!SourceType.workerSourceURL) {
return;
}
const dispatcher = new Dispatcher(getGlobalWorkerPool(), 'add-custom-source-dispatcher');
await dispatcher.broadcast('loadWorkerSource', SourceType.workerSourceURL.toString());
dispatcher.remove();
};
4 changes: 2 additions & 2 deletions src/source/source_cache.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {SourceCache} from './source_cache';
import {Source, setSourceType} from './source';
import {Source, addSourceType} from './source';
import {Tile} from './tile';
import {OverscaledTileID} from './tile_id';
import {Transform} from '../geo/transform';
Expand Down Expand Up @@ -75,7 +75,7 @@ function createSource(id: string, sourceOptions: any, _dispatcher: any, eventedP
return source;
}

setSourceType('mock-source-type', createSource as any);
addSourceType('mock-source-type', createSource as any);

function createSourceCache(options?, used?) {
const sc = new SourceCache('id', extend({
Expand Down
47 changes: 0 additions & 47 deletions src/style/style.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {fakeServer, type FakeServer} from 'nise';

import {EvaluationParameters} from './evaluation_parameters';
import {LayerSpecification, GeoJSONSourceSpecification, FilterSpecification, SourceSpecification} from '@maplibre/maplibre-gl-style-spec';
import {SourceClass} from '../source/source';
import {GeoJSONSource} from '../source/geojson_source';

function createStyleJSON(properties?) {
Expand Down Expand Up @@ -2503,52 +2502,6 @@ describe('Style#query*Features', () => {
});
});

describe('Style#addSourceType', () => {
test('adds factory function', done => {
const style = new Style(getStubMap());
const sourceType = function () {} as any as SourceClass;

// expect no call to load worker source
style.dispatcher.broadcast = function (type) {
if (type === 'loadWorkerSource') {
done('test failed');
}
return Promise.resolve({} as any);
};

style.addSourceType('foo', sourceType, (arg1, arg2) => {
expect(arg1).toBeNull();
expect(arg2).toBeNull();
done();
});
});

test('triggers workers to load worker source code', done => {
const style = new Style(getStubMap());
const sourceType = function () {} as any as SourceClass;
sourceType.workerSourceURL = 'worker-source.js' as any as URL;

style.dispatcher.broadcast = (type, params) => {
if (type === 'loadWorkerSource') {
expect(params).toBe('worker-source.js');
done();
return Promise.resolve({} as any);
}
};

style.addSourceType('bar', sourceType, (err) => { expect(err).toBeFalsy(); });
});

test('refuses to add new type over existing name', done => {
const style = new Style(getStubMap());
const sourceType = function () {} as any as SourceClass;
style.addSourceType('canvas', sourceType, (err) => {
expect(err).toBeTruthy();
done();
});
});
});

describe('Style#hasTransitions', () => {
test('returns false when the style is loading', () => {
const style = new Style(getStubMap());
Expand Down
18 changes: 1 addition & 17 deletions src/style/style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ import {ResourceType} from '../util/request_manager';
import {browser} from '../util/browser';
import {Dispatcher} from '../util/dispatcher';
import {validateStyle, emitValidationErrors as _emitValidationErrors} from './validate_style';
import {getSourceType, setSourceType, Source} from '../source/source';
import type {SourceClass} from '../source/source';
import {Source} from '../source/source';
import {QueryRenderedFeaturesOptions, QuerySourceFeatureOptions, queryRenderedFeatures, queryRenderedSymbols, querySourceFeatures} from '../source/query_features';
import {SourceCache} from '../source/source_cache';
import {GeoJSONSource} from '../source/geojson_source';
Expand Down Expand Up @@ -43,7 +42,6 @@ const emitValidationErrors = (evented: Evented, errors?: ReadonlyArray<{
import type {Map} from '../ui/map';
import type {Transform} from '../geo/transform';
import type {StyleImage} from './style_image';
import type {Callback} from '../types/callback';
import type {EvaluationParameters} from './evaluation_parameters';
import type {Placement} from '../symbol/placement';
import type {GetResourceResponse, RequestParameters} from '../util/ajax';
Expand Down Expand Up @@ -1415,20 +1413,6 @@ export class Style extends Evented {
return sourceCache ? querySourceFeatures(sourceCache, params) : [];
}

addSourceType(name: string, SourceType: SourceClass, callback: Callback<void>) {
if (getSourceType(name)) {
return callback(new Error(`A source type called "${name}" already exists.`));
}

setSourceType(name, SourceType);

if (!SourceType.workerSourceURL) {
return callback(null, null);
}

this.dispatcher.broadcast('loadWorkerSource', SourceType.workerSourceURL.toString()).then(() => callback()).catch(callback);
}

getLight() {
return this.light.getLight();
}
Expand Down
15 changes: 1 addition & 14 deletions src/ui/map.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {TaskQueue} from '../util/task_queue';
import {throttle} from '../util/throttle';
import {webpSupported} from '../util/webp_supported';
import {PerformanceMarkers, PerformanceUtils} from '../util/performance';
import {Source, SourceClass} from '../source/source';
import {Source} from '../source/source';
import {StyleLayer} from '../style/style_layer';

import type {RequestTransformFunction} from '../util/request_manager';
Expand Down Expand Up @@ -57,7 +57,6 @@ import type {
TerrainSpecification
} from '@maplibre/maplibre-gl-style-spec';

import {Callback} from '../types/callback';
import type {ControlPosition, IControl} from './control/control';
import type {MapGeoJSONFeature} from '../util/vectortile_to_geojson';
import {Terrain} from '../render/terrain';
Expand Down Expand Up @@ -2044,18 +2043,6 @@ export class Map extends Camera {
return true;
}

/**
* Adds a [custom source type](#Custom Sources), making it available for use with
* {@link Map#addSource}.
* @param name - The name of the source type; source definition objects use this name in the `{type: ...}` field.
* @param SourceType - A {@link Source} constructor.
* @param callback - Called when the source type is ready or with an error argument if there is an error.
*/
addSourceType(name: string, SourceType: SourceClass, callback: Callback<void>) {
this._lazyInitEmptyStyle();
return this.style.addSourceType(name, SourceType, callback);
}

/**
* Removes a source from the map's style.
*
Expand Down
Loading