From 7667d805452fb16cdb71d47f7c0bb0e4df4eb9e9 Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Fri, 31 May 2024 12:57:39 +0200 Subject: [PATCH 01/16] add failing unit test for ajaxError serialization not working - all extendend properties get lost --- jest.config.ts | 2 +- jest.jsdom.environment.ts | 10 ++++++++++ src/util/web_worker_transfer.test.ts | 18 ++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 jest.jsdom.environment.ts diff --git a/jest.config.ts b/jest.config.ts index 58baa44515..5684e52c1e 100644 --- a/jest.config.ts +++ b/jest.config.ts @@ -35,7 +35,7 @@ const config: Config = { projects: [ { displayName: 'unit', - testEnvironment: 'jsdom', + testEnvironment: './jest.jsdom.environment.ts', setupFiles: [ 'jest-webgl-canvas-mock', './test/unit/lib/web_worker_mock.ts' diff --git a/jest.jsdom.environment.ts b/jest.jsdom.environment.ts new file mode 100644 index 0000000000..6248558eb9 --- /dev/null +++ b/jest.jsdom.environment.ts @@ -0,0 +1,10 @@ +import JSDOMEnvironment from 'jest-environment-jsdom'; + +export default class FixJSDOMEnvironment extends JSDOMEnvironment { + constructor(...args: ConstructorParameters) { + super(...args); + + // https://github.com/jsdom/jsdom/issues/3363 + this.global.structuredClone = structuredClone; + } +} diff --git a/src/util/web_worker_transfer.test.ts b/src/util/web_worker_transfer.test.ts index f8fc7a9ebc..abcb4837ef 100644 --- a/src/util/web_worker_transfer.test.ts +++ b/src/util/web_worker_transfer.test.ts @@ -1,5 +1,11 @@ +import {AJAXError} from './ajax'; import {register, serialize, deserialize} from './web_worker_transfer'; +const mockTransfer = (input, transferables?) => { + const serialized = serialize(input, transferables); + return deserialize(structuredClone(serialized, {transfer: transferables})); +}; + describe('web worker transfer', () => { test('round trip', () => { class SerializableMock { @@ -80,4 +86,16 @@ describe('web worker transfer', () => { expect(deserialized.id).toBe(customSerialization.id); expect(deserialized._deserialized).toBeTruthy(); }); + + test('AjaxError serialization', () => { + const status = 404; + const statusText = 'not found'; + const url = 'https://example.com'; + + const ajaxError = new AJAXError(status, statusText, url, new Blob()); + const deserialized = mockTransfer(ajaxError) as AJAXError; + expect(deserialized.status).toBe(404); + expect(deserialized.statusText).toBe(statusText); + expect(deserialized.url).toBe(url); + }); }); From cbf397dda15e5d1ace903670acd67fe5667984df Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Fri, 31 May 2024 12:58:29 +0200 Subject: [PATCH 02/16] change all worker_transfer tests to use structuredClone for realistic test; note that jsdom Blob does not work with node native structuredClone so that one line is disabled --- src/util/web_worker_transfer.test.ts | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/util/web_worker_transfer.test.ts b/src/util/web_worker_transfer.test.ts index abcb4837ef..63cc4fa855 100644 --- a/src/util/web_worker_transfer.test.ts +++ b/src/util/web_worker_transfer.test.ts @@ -17,7 +17,7 @@ describe('web worker transfer', () => { constructor(n) { this.n = n; this.buffer = new ArrayBuffer(100); - this.blob = new Blob(); + this.blob = new Blob(['Test'], {type: 'application/text'}); this.squared(); } @@ -34,15 +34,20 @@ describe('web worker transfer', () => { const serializableMock = new SerializableMock(10); const transferables = []; - const deserialized = deserialize(serialize(serializableMock, transferables)) as SerializableMock; - expect(deserialize(serialize(serializableMock, transferables)) instanceof SerializableMock).toBeTruthy(); - + const deserialized = mockTransfer(serializableMock, transferables) as SerializableMock; + expect(deserialized instanceof SerializableMock).toBeTruthy(); + expect(transferables[0] === serializableMock.buffer).toBeTruthy(); + expect(serializableMock.buffer.byteLength).toBe(0); + expect(deserialized.buffer.byteLength).toBe(100); expect(serializableMock !== deserialized).toBeTruthy(); expect(deserialized.constructor === SerializableMock).toBeTruthy(); expect(deserialized.n === 10).toBeTruthy(); - expect(deserialized.buffer === serializableMock.buffer).toBeTruthy(); - expect(deserialized.blob === serializableMock.blob).toBeTruthy(); - expect(transferables[0] === serializableMock.buffer).toBeTruthy(); + expect(serializableMock.blob.size).toBe(4); + // seems to be a problem with jsdom + node. it works in + // node and it works in browsers + // expect(structuredClone(new Blob())).toBeInstanceOf(Blob); + // expect(deserialized.blob.size).toBe(4); + expect(deserialized._cached === undefined).toBeTruthy(); expect(deserialized.squared() === 100).toBeTruthy(); }); @@ -52,7 +57,7 @@ describe('web worker transfer', () => { expect(!Klass.name).toBeTruthy(); register('Anon', Klass); const x = new Klass(); - const deserialized = deserialize(serialize(x)); + const deserialized = mockTransfer(x); expect(deserialized instanceof Klass).toBeTruthy(); }); @@ -81,8 +86,8 @@ describe('web worker transfer', () => { const customSerialization = new CustomSerialization('a'); expect(!customSerialization._deserialized).toBeTruthy(); - const deserialized = deserialize(serialize(customSerialization)) as CustomSerialization; - expect(deserialize(serialize(customSerialization)) instanceof CustomSerialization).toBeTruthy(); + const deserialized = mockTransfer(customSerialization) as CustomSerialization; + expect(mockTransfer(customSerialization) instanceof CustomSerialization).toBeTruthy(); expect(deserialized.id).toBe(customSerialization.id); expect(deserialized._deserialized).toBeTruthy(); }); From 74560b907038fab872d793a977fc7199fa7617aa Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Fri, 31 May 2024 13:13:28 +0200 Subject: [PATCH 03/16] serialize/deserialize Error so extends of Error pass through postMessage; e.g. AJAXError which already is registered for transfer but ignored because it's instance of Error --- src/util/web_worker_transfer.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/util/web_worker_transfer.ts b/src/util/web_worker_transfer.ts index 8a2319212f..83952a0d42 100644 --- a/src/util/web_worker_transfer.ts +++ b/src/util/web_worker_transfer.ts @@ -114,8 +114,7 @@ export function serialize(input: unknown, transferables?: Array | input instanceof String || input instanceof Date || input instanceof RegExp || - input instanceof Blob || - input instanceof Error) { + input instanceof Blob) { return input; } @@ -217,7 +216,6 @@ export function deserialize(input: Serialized): unknown { input instanceof Date || input instanceof RegExp || input instanceof Blob || - input instanceof Error || isArrayBuffer(input) || isImageBitmap(input) || ArrayBuffer.isView(input) || From 82800258820af5ee9e36ecfc01d0ab05a3f60ec2 Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Fri, 31 May 2024 13:22:31 +0200 Subject: [PATCH 04/16] changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d93ec7fc0..4e9721b6e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ ### 🐞 Bug fixes - ⚠️ Allow breaking lines in labels before a left parenthesis ([#4138](https://github.com/maplibre/maplibre-gl-js/pull/4138)) - ⚠️ Fix ignoring embedded line breaks when `symbol-placement` is `line` or `line-center` ([#4124](https://github.com/maplibre/maplibre-gl-js/pull/4124)) +- Fix AjaxError not properly serialized ([#4024](https://github.com/maplibre/maplibre-gl-js/pull/4211)) - _...Add new stuff here..._ ## 4.3.2 From 5f378cdb708039766ed1fe1b2be494db3f0c56ce Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Mon, 3 Jun 2024 12:19:47 +0200 Subject: [PATCH 05/16] if the sublcass of a built-in object is registered, use the registered class, otherwise use the built-in to serialize/deserialize; fixes the issue that most Errors are (de)serialized by the builtin but AjaxError extend is not --- src/util/web_worker_transfer.ts | 47 ++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/src/util/web_worker_transfer.ts b/src/util/web_worker_transfer.ts index 83952a0d42..df01e63bfd 100644 --- a/src/util/web_worker_transfer.ts +++ b/src/util/web_worker_transfer.ts @@ -91,6 +91,15 @@ function isArrayBuffer(value: any): value is ArrayBuffer { (value instanceof ArrayBuffer || (value.constructor && value.constructor.name === 'ArrayBuffer')); } +function getRegisterName(input: unknown) : string|undefined { + if (typeof input !== 'object') { + return; + } + const klass = (input.constructor as any); + const name = klass._classRegistryKey; + return name; +} + /** * Serialize the given object for transfer to or from a web worker. * @@ -104,17 +113,22 @@ function isArrayBuffer(value: any): value is ArrayBuffer { * this should happen in the client code, before using serialize().) */ export function serialize(input: unknown, transferables?: Array | null): Serialized { + const name = getRegisterName(input); if (input === null || input === undefined || typeof input === 'boolean' || typeof input === 'number' || typeof input === 'string' || - input instanceof Boolean || - input instanceof Number || - input instanceof String || - input instanceof Date || - input instanceof RegExp || - input instanceof Blob) { + (!name && + (input instanceof Boolean || + input instanceof Number || + input instanceof String || + input instanceof Date || + input instanceof RegExp || + input instanceof Blob || + input instanceof Error + ) + )) { return input; } @@ -157,7 +171,6 @@ export function serialize(input: unknown, transferables?: Array | if (typeof input === 'object') { const klass = (input.constructor as any); - const name = klass._classRegistryKey; if (!name) { throw new Error(`can't serialize object of unregistered class ${klass.name}`); } @@ -205,21 +218,25 @@ export function serialize(input: unknown, transferables?: Array | } export function deserialize(input: Serialized): unknown { + const hasRegisterName = typeof input === 'object' && (input).$name; if (input === null || input === undefined || typeof input === 'boolean' || typeof input === 'number' || typeof input === 'string' || - input instanceof Boolean || - input instanceof Number || - input instanceof String || - input instanceof Date || - input instanceof RegExp || - input instanceof Blob || isArrayBuffer(input) || isImageBitmap(input) || ArrayBuffer.isView(input) || - input instanceof ImageData) { + (!hasRegisterName && ( + input instanceof ImageData || + input instanceof Error || + input instanceof Boolean || + input instanceof Number || + input instanceof String || + input instanceof Date || + input instanceof RegExp || + input instanceof Blob + ))) { return input; } @@ -228,7 +245,7 @@ export function deserialize(input: Serialized): unknown { } if (typeof input === 'object') { - const name = input.$name || 'Object'; + const name = (input).$name || 'Object'; if (!registry[name]) { throw new Error(`can't deserialize unregistered class ${name}`); } From e468fe5cd75d35508ea001e97f7807d09eaad18d Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Tue, 4 Jun 2024 08:52:43 +0200 Subject: [PATCH 06/16] Revert "if the sublcass of a built-in object is registered, use the registered class, otherwise use the built-in to serialize/deserialize; fixes the issue that most Errors are (de)serialized by the builtin but AjaxError extend is not" This reverts commit 5f378cdb708039766ed1fe1b2be494db3f0c56ce. --- src/util/web_worker_transfer.ts | 47 +++++++++++---------------------- 1 file changed, 15 insertions(+), 32 deletions(-) diff --git a/src/util/web_worker_transfer.ts b/src/util/web_worker_transfer.ts index df01e63bfd..83952a0d42 100644 --- a/src/util/web_worker_transfer.ts +++ b/src/util/web_worker_transfer.ts @@ -91,15 +91,6 @@ function isArrayBuffer(value: any): value is ArrayBuffer { (value instanceof ArrayBuffer || (value.constructor && value.constructor.name === 'ArrayBuffer')); } -function getRegisterName(input: unknown) : string|undefined { - if (typeof input !== 'object') { - return; - } - const klass = (input.constructor as any); - const name = klass._classRegistryKey; - return name; -} - /** * Serialize the given object for transfer to or from a web worker. * @@ -113,22 +104,17 @@ function getRegisterName(input: unknown) : string|undefined { * this should happen in the client code, before using serialize().) */ export function serialize(input: unknown, transferables?: Array | null): Serialized { - const name = getRegisterName(input); if (input === null || input === undefined || typeof input === 'boolean' || typeof input === 'number' || typeof input === 'string' || - (!name && - (input instanceof Boolean || - input instanceof Number || - input instanceof String || - input instanceof Date || - input instanceof RegExp || - input instanceof Blob || - input instanceof Error - ) - )) { + input instanceof Boolean || + input instanceof Number || + input instanceof String || + input instanceof Date || + input instanceof RegExp || + input instanceof Blob) { return input; } @@ -171,6 +157,7 @@ export function serialize(input: unknown, transferables?: Array | if (typeof input === 'object') { const klass = (input.constructor as any); + const name = klass._classRegistryKey; if (!name) { throw new Error(`can't serialize object of unregistered class ${klass.name}`); } @@ -218,25 +205,21 @@ export function serialize(input: unknown, transferables?: Array | } export function deserialize(input: Serialized): unknown { - const hasRegisterName = typeof input === 'object' && (input).$name; if (input === null || input === undefined || typeof input === 'boolean' || typeof input === 'number' || typeof input === 'string' || + input instanceof Boolean || + input instanceof Number || + input instanceof String || + input instanceof Date || + input instanceof RegExp || + input instanceof Blob || isArrayBuffer(input) || isImageBitmap(input) || ArrayBuffer.isView(input) || - (!hasRegisterName && ( - input instanceof ImageData || - input instanceof Error || - input instanceof Boolean || - input instanceof Number || - input instanceof String || - input instanceof Date || - input instanceof RegExp || - input instanceof Blob - ))) { + input instanceof ImageData) { return input; } @@ -245,7 +228,7 @@ export function deserialize(input: Serialized): unknown { } if (typeof input === 'object') { - const name = (input).$name || 'Object'; + const name = input.$name || 'Object'; if (!registry[name]) { throw new Error(`can't deserialize unregistered class ${name}`); } From 5b19589936d63f5eb8fd0c18e23cb717b09a368f Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Tue, 4 Jun 2024 09:55:35 +0200 Subject: [PATCH 07/16] when serializing/deserializing check if the class is registered --- src/util/web_worker_transfer.ts | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/src/util/web_worker_transfer.ts b/src/util/web_worker_transfer.ts index 83952a0d42..d9e7f4116f 100644 --- a/src/util/web_worker_transfer.ts +++ b/src/util/web_worker_transfer.ts @@ -91,6 +91,20 @@ function isArrayBuffer(value: any): value is ArrayBuffer { (value instanceof ArrayBuffer || (value.constructor && value.constructor.name === 'ArrayBuffer')); } +function isRegistered(input: unknown) { + if (input === null || typeof input !== 'object') { + return false; + } + const klass = (input.constructor as any); + if (klass._classRegistryKey && klass._classRegistryKey !== 'Object') { + return true; + } + if ((input).$name) { + return true; + } + return false; +} + /** * Serialize the given object for transfer to or from a web worker. * @@ -104,7 +118,8 @@ function isArrayBuffer(value: any): value is ArrayBuffer { * this should happen in the client code, before using serialize().) */ export function serialize(input: unknown, transferables?: Array | null): Serialized { - if (input === null || + if (!isRegistered(input) && ( + input === null || input === undefined || typeof input === 'boolean' || typeof input === 'number' || @@ -114,7 +129,9 @@ export function serialize(input: unknown, transferables?: Array | input instanceof String || input instanceof Date || input instanceof RegExp || - input instanceof Blob) { + input instanceof Blob || + input instanceof Error) + ) { return input; } @@ -205,7 +222,8 @@ export function serialize(input: unknown, transferables?: Array | } export function deserialize(input: Serialized): unknown { - if (input === null || + if (!isRegistered(input) && ( + input === null || input === undefined || typeof input === 'boolean' || typeof input === 'number' || @@ -219,7 +237,9 @@ export function deserialize(input: Serialized): unknown { isArrayBuffer(input) || isImageBitmap(input) || ArrayBuffer.isView(input) || - input instanceof ImageData) { + input instanceof ImageData || + input instanceof Error + )) { return input; } @@ -228,7 +248,7 @@ export function deserialize(input: Serialized): unknown { } if (typeof input === 'object') { - const name = input.$name || 'Object'; + const name = (input).$name || 'Object'; if (!registry[name]) { throw new Error(`can't deserialize unregistered class ${name}`); } From 1374644017b439854a4bd1ab5246fe878fe27266 Mon Sep 17 00:00:00 2001 From: Simon Oberhammer <53384+oberhamsi@users.noreply.github.com> Date: Tue, 4 Jun 2024 10:41:15 +0200 Subject: [PATCH 08/16] style fix as reviewed Co-authored-by: Harel M --- src/util/web_worker_transfer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/web_worker_transfer.ts b/src/util/web_worker_transfer.ts index d9e7f4116f..2408952343 100644 --- a/src/util/web_worker_transfer.ts +++ b/src/util/web_worker_transfer.ts @@ -99,7 +99,7 @@ function isRegistered(input: unknown) { if (klass._classRegistryKey && klass._classRegistryKey !== 'Object') { return true; } - if ((input).$name) { + if ((input as SerializedObject).$name) { return true; } return false; From 7fd8cab80b911c75533c557c8f00412d0ae1d0e7 Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Wed, 5 Jun 2024 12:39:25 +0200 Subject: [PATCH 09/16] refactor for less duplication --- src/util/web_worker_transfer.ts | 228 +++++++++++++++----------------- 1 file changed, 103 insertions(+), 125 deletions(-) diff --git a/src/util/web_worker_transfer.ts b/src/util/web_worker_transfer.ts index 2408952343..8cded07324 100644 --- a/src/util/web_worker_transfer.ts +++ b/src/util/web_worker_transfer.ts @@ -21,6 +21,7 @@ type Registry = { klass: { new (...args: any): any; deserialize?: (input: Serialized) => unknown; + serialize?: (input: any, transferables: Transferable[]) => SerializedObject; }; omit: ReadonlyArray; shallow: ReadonlyArray; @@ -91,20 +92,43 @@ function isArrayBuffer(value: any): value is ArrayBuffer { (value instanceof ArrayBuffer || (value.constructor && value.constructor.name === 'ArrayBuffer')); } +function getClassRegistryKey(input: Object|SerializedObject) { + const klass = (input.constructor as any); + return (input as SerializedObject).$name || klass._classRegistryKey; +} + function isRegistered(input: unknown) { if (input === null || typeof input !== 'object') { return false; } - const klass = (input.constructor as any); - if (klass._classRegistryKey && klass._classRegistryKey !== 'Object') { - return true; - } - if ((input as SerializedObject).$name) { + const classRegistryKey = getClassRegistryKey(input); + if (classRegistryKey && classRegistryKey !== 'Object') { return true; } return false; } +function isSerializeHandledByBuiltin(input: unknown) { + return (!isRegistered(input) && ( + input === null || + input === undefined || + typeof input === 'boolean' || + typeof input === 'number' || + typeof input === 'string' || + input instanceof Boolean || + input instanceof Number || + input instanceof String || + input instanceof Date || + input instanceof RegExp || + input instanceof Blob || + input instanceof Error || + isArrayBuffer(input) || + isImageBitmap(input) || + ArrayBuffer.isView(input) || + input instanceof ImageData) + ); +} + /** * Serialize the given object for transfer to or from a web worker. * @@ -118,48 +142,22 @@ function isRegistered(input: unknown) { * this should happen in the client code, before using serialize().) */ export function serialize(input: unknown, transferables?: Array | null): Serialized { - if (!isRegistered(input) && ( - input === null || - input === undefined || - typeof input === 'boolean' || - typeof input === 'number' || - typeof input === 'string' || - input instanceof Boolean || - input instanceof Number || - input instanceof String || - input instanceof Date || - input instanceof RegExp || - input instanceof Blob || - input instanceof Error) - ) { - return input; - } - - if (isArrayBuffer(input)) { - if (transferables) { - transferables.push(input); - } - return input; - } - - if (isImageBitmap(input)) { - if (transferables) { - transferables.push(input); + if (isSerializeHandledByBuiltin(input)) { + if (isArrayBuffer(input) || isImageBitmap(input)) { + if (transferables) { + transferables.push(input); + } } - return input; - } - - if (ArrayBuffer.isView(input)) { - const view = input; - if (transferables) { - transferables.push(view.buffer); + if (ArrayBuffer.isView(input)) { + const view = input; + if (transferables) { + transferables.push(view.buffer); + } } - return view; - } - - if (input instanceof ImageData) { - if (transferables) { - transferables.push(input.data.buffer); + if (input instanceof ImageData) { + if (transferables) { + transferables.push(input.data.buffer); + } } return input; } @@ -172,74 +170,55 @@ export function serialize(input: unknown, transferables?: Array | return serialized; } - if (typeof input === 'object') { - const klass = (input.constructor as any); - const name = klass._classRegistryKey; - if (!name) { - throw new Error(`can't serialize object of unregistered class ${klass.name}`); - } - if (!registry[name]) throw new Error(`${name} is not registered.`); - - const properties: SerializedObject = klass.serialize ? - // (Temporary workaround) allow a class to provide static - // `serialize()` and `deserialize()` methods to bypass the generic - // approach. - // This temporary workaround lets us use the generic serialization - // approach for objects whose members include instances of dynamic - // StructArray types. Once we refactor StructArray to be static, - // we can remove this complexity. - (klass.serialize(input, transferables) as SerializedObject) : {}; - - if (!klass.serialize) { - for (const key in input) { - if (!input.hasOwnProperty(key)) continue; // eslint-disable-line no-prototype-builtins - if (registry[name].omit.indexOf(key) >= 0) continue; - const property = input[key]; - properties[key] = registry[name].shallow.indexOf(key) >= 0 ? - property : - serialize(property, transferables); - } - if (input instanceof Error) { - properties.message = input.message; - } - } else { - if (transferables && properties === transferables[transferables.length - 1]) { - throw new Error('statically serialized object won\'t survive transfer of $name property'); - } + if (typeof input !== 'object') { + throw new Error(`can't serialize object of type ${typeof input}`); + } + const classRegistryKey = getClassRegistryKey(input); + if (!classRegistryKey) { + throw new Error(`can't serialize object of unregistered class ${input.constructor.name}`); + } + if (!registry[classRegistryKey]) throw new Error(`${classRegistryKey} is not registered.`); + const {klass} = registry[classRegistryKey]; + const properties: SerializedObject = klass.serialize ? + // (Temporary workaround) allow a class to provide static + // `serialize()` and `deserialize()` methods to bypass the generic + // approach. + // This temporary workaround lets us use the generic serialization + // approach for objects whose members include instances of dynamic + // StructArray types. Once we refactor StructArray to be static, + // we can remove this complexity. + (klass.serialize(input, transferables) as SerializedObject) : {}; + + if (!klass.serialize) { + for (const key in input) { + if (!input.hasOwnProperty(key)) continue; // eslint-disable-line no-prototype-builtins + if (registry[classRegistryKey].omit.indexOf(key) >= 0) continue; + const property = input[key]; + properties[key] = registry[classRegistryKey].shallow.indexOf(key) >= 0 ? + property : + serialize(property, transferables); } - - if (properties.$name) { - throw new Error('$name property is reserved for worker serialization logic.'); + if (input instanceof Error) { + properties.message = input.message; } - if (name !== 'Object') { - properties.$name = name; + } else { + if (transferables && properties === transferables[transferables.length - 1]) { + throw new Error('statically serialized object won\'t survive transfer of $name property'); } + } - return properties; + if (properties.$name) { + throw new Error('$name property is reserved for worker serialization logic.'); + } + if (classRegistryKey !== 'Object') { + properties.$name = classRegistryKey; } - throw new Error(`can't serialize object of type ${typeof input}`); + return properties; } export function deserialize(input: Serialized): unknown { - if (!isRegistered(input) && ( - input === null || - input === undefined || - typeof input === 'boolean' || - typeof input === 'number' || - typeof input === 'string' || - input instanceof Boolean || - input instanceof Number || - input instanceof String || - input instanceof Date || - input instanceof RegExp || - input instanceof Blob || - isArrayBuffer(input) || - isImageBitmap(input) || - ArrayBuffer.isView(input) || - input instanceof ImageData || - input instanceof Error - )) { + if (isSerializeHandledByBuiltin(input)) { return input; } @@ -247,30 +226,29 @@ export function deserialize(input: Serialized): unknown { return input.map(deserialize); } - if (typeof input === 'object') { - const name = (input).$name || 'Object'; - if (!registry[name]) { - throw new Error(`can't deserialize unregistered class ${name}`); - } - const {klass} = registry[name]; - if (!klass) { - throw new Error(`can't deserialize unregistered class ${name}`); - } - - if (klass.deserialize) { - return klass.deserialize(input); - } + if (typeof input !== 'object') { + throw new Error(`can't deserialize object of type ${typeof input}`); + } + const classRegistryKey = getClassRegistryKey(input) || 'Object'; + if (!registry[classRegistryKey]) { + throw new Error(`can't deserialize unregistered class ${classRegistryKey}`); + } + const {klass} = registry[classRegistryKey]; + if (!klass) { + throw new Error(`can't deserialize unregistered class ${classRegistryKey}`); + } - const result = Object.create(klass.prototype); + if (klass.deserialize) { + return klass.deserialize(input); + } - for (const key of Object.keys(input)) { - if (key === '$name') continue; - const value = (input as SerializedObject)[key]; - result[key] = registry[name].shallow.indexOf(key) >= 0 ? value : deserialize(value); - } + const result = Object.create(klass.prototype); - return result; + for (const key of Object.keys(input)) { + if (key === '$name') continue; + const value = (input as SerializedObject)[key]; + result[key] = registry[classRegistryKey].shallow.indexOf(key) >= 0 ? value : deserialize(value); } - throw new Error(`can't deserialize object of type ${typeof input}`); + return result; } From 53ab06f3d00fc0e71d250113d5278090ad415576 Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Wed, 5 Jun 2024 12:48:13 +0200 Subject: [PATCH 10/16] Revert "change all worker_transfer tests to use structuredClone for realistic test; note that jsdom Blob does not work with node native structuredClone so that one line is disabled" This reverts commit cbf397dda15e5d1ace903670acd67fe5667984df. --- src/util/web_worker_transfer.test.ts | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/util/web_worker_transfer.test.ts b/src/util/web_worker_transfer.test.ts index 63cc4fa855..abcb4837ef 100644 --- a/src/util/web_worker_transfer.test.ts +++ b/src/util/web_worker_transfer.test.ts @@ -17,7 +17,7 @@ describe('web worker transfer', () => { constructor(n) { this.n = n; this.buffer = new ArrayBuffer(100); - this.blob = new Blob(['Test'], {type: 'application/text'}); + this.blob = new Blob(); this.squared(); } @@ -34,20 +34,15 @@ describe('web worker transfer', () => { const serializableMock = new SerializableMock(10); const transferables = []; - const deserialized = mockTransfer(serializableMock, transferables) as SerializableMock; - expect(deserialized instanceof SerializableMock).toBeTruthy(); - expect(transferables[0] === serializableMock.buffer).toBeTruthy(); - expect(serializableMock.buffer.byteLength).toBe(0); - expect(deserialized.buffer.byteLength).toBe(100); + const deserialized = deserialize(serialize(serializableMock, transferables)) as SerializableMock; + expect(deserialize(serialize(serializableMock, transferables)) instanceof SerializableMock).toBeTruthy(); + expect(serializableMock !== deserialized).toBeTruthy(); expect(deserialized.constructor === SerializableMock).toBeTruthy(); expect(deserialized.n === 10).toBeTruthy(); - expect(serializableMock.blob.size).toBe(4); - // seems to be a problem with jsdom + node. it works in - // node and it works in browsers - // expect(structuredClone(new Blob())).toBeInstanceOf(Blob); - // expect(deserialized.blob.size).toBe(4); - + expect(deserialized.buffer === serializableMock.buffer).toBeTruthy(); + expect(deserialized.blob === serializableMock.blob).toBeTruthy(); + expect(transferables[0] === serializableMock.buffer).toBeTruthy(); expect(deserialized._cached === undefined).toBeTruthy(); expect(deserialized.squared() === 100).toBeTruthy(); }); @@ -57,7 +52,7 @@ describe('web worker transfer', () => { expect(!Klass.name).toBeTruthy(); register('Anon', Klass); const x = new Klass(); - const deserialized = mockTransfer(x); + const deserialized = deserialize(serialize(x)); expect(deserialized instanceof Klass).toBeTruthy(); }); @@ -86,8 +81,8 @@ describe('web worker transfer', () => { const customSerialization = new CustomSerialization('a'); expect(!customSerialization._deserialized).toBeTruthy(); - const deserialized = mockTransfer(customSerialization) as CustomSerialization; - expect(mockTransfer(customSerialization) instanceof CustomSerialization).toBeTruthy(); + const deserialized = deserialize(serialize(customSerialization)) as CustomSerialization; + expect(deserialize(serialize(customSerialization)) instanceof CustomSerialization).toBeTruthy(); expect(deserialized.id).toBe(customSerialization.id); expect(deserialized._deserialized).toBeTruthy(); }); From 05ae5e8daeefb2fd55907e116abeadcec7b8d6ac Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Wed, 5 Jun 2024 12:55:02 +0200 Subject: [PATCH 11/16] change the ajaxerror test to not use structuredClone until jsdom provides it since we don't want a custom jest env --- jest.config.ts | 2 +- jest.jsdom.environment.ts | 10 ---------- src/util/web_worker_transfer.test.ts | 10 ++++------ 3 files changed, 5 insertions(+), 17 deletions(-) delete mode 100644 jest.jsdom.environment.ts diff --git a/jest.config.ts b/jest.config.ts index 5684e52c1e..58baa44515 100644 --- a/jest.config.ts +++ b/jest.config.ts @@ -35,7 +35,7 @@ const config: Config = { projects: [ { displayName: 'unit', - testEnvironment: './jest.jsdom.environment.ts', + testEnvironment: 'jsdom', setupFiles: [ 'jest-webgl-canvas-mock', './test/unit/lib/web_worker_mock.ts' diff --git a/jest.jsdom.environment.ts b/jest.jsdom.environment.ts deleted file mode 100644 index 6248558eb9..0000000000 --- a/jest.jsdom.environment.ts +++ /dev/null @@ -1,10 +0,0 @@ -import JSDOMEnvironment from 'jest-environment-jsdom'; - -export default class FixJSDOMEnvironment extends JSDOMEnvironment { - constructor(...args: ConstructorParameters) { - super(...args); - - // https://github.com/jsdom/jsdom/issues/3363 - this.global.structuredClone = structuredClone; - } -} diff --git a/src/util/web_worker_transfer.test.ts b/src/util/web_worker_transfer.test.ts index abcb4837ef..ae283023f7 100644 --- a/src/util/web_worker_transfer.test.ts +++ b/src/util/web_worker_transfer.test.ts @@ -1,11 +1,7 @@ +import {SerializedObject} from '../../dist/maplibre-gl'; import {AJAXError} from './ajax'; import {register, serialize, deserialize} from './web_worker_transfer'; -const mockTransfer = (input, transferables?) => { - const serialized = serialize(input, transferables); - return deserialize(structuredClone(serialized, {transfer: transferables})); -}; - describe('web worker transfer', () => { test('round trip', () => { class SerializableMock { @@ -93,7 +89,9 @@ describe('web worker transfer', () => { const url = 'https://example.com'; const ajaxError = new AJAXError(status, statusText, url, new Blob()); - const deserialized = mockTransfer(ajaxError) as AJAXError; + const serialized = serialize(ajaxError) as SerializedObject; + expect(serialized.$name).toBe(ajaxError.constructor.name); + const deserialized = deserialize(serialized) as AJAXError; expect(deserialized.status).toBe(404); expect(deserialized.statusText).toBe(statusText); expect(deserialized.url).toBe(url); From ec4900c738dc7bb5bec0a1848787f2ae6df388c0 Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Wed, 5 Jun 2024 12:58:35 +0200 Subject: [PATCH 12/16] improve changelog entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8598bb6e8c..1baee158ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ - ⚠️ Allow breaking lines in labels before a left parenthesis ([#4138](https://github.com/maplibre/maplibre-gl-js/pull/4138)) - ⚠️ Fix ignoring embedded line breaks when `symbol-placement` is `line` or `line-center` ([#4124](https://github.com/maplibre/maplibre-gl-js/pull/4124)) -- Fix AjaxError not properly serialized ([#4024](https://github.com/maplibre/maplibre-gl-js/pull/4211)) +- Fix extends of built-ins (currently only AjaxError) not properly getting (de)serialized ([#4024](https://github.com/maplibre/maplibre-gl-js/pull/4211)) - _...Add new stuff here..._ ## 4.3.2 From 0146720a3b9037bccc765e7e0a20f7c9fdf5c1f0 Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Wed, 5 Jun 2024 14:13:21 +0200 Subject: [PATCH 13/16] add more tests to web_worker_transfer --- src/util/web_worker_transfer.test.ts | 29 ++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/util/web_worker_transfer.test.ts b/src/util/web_worker_transfer.test.ts index ae283023f7..81f59fd746 100644 --- a/src/util/web_worker_transfer.test.ts +++ b/src/util/web_worker_transfer.test.ts @@ -9,11 +9,17 @@ describe('web worker transfer', () => { buffer; blob; _cached; + dataView; + imageData; + array; constructor(n) { this.n = n; this.buffer = new ArrayBuffer(100); + this.dataView = new DataView(this.buffer); + this.imageData = new ImageData(8, 5); this.blob = new Blob(); + this.array = [true, 1, 'one', new ArrayBuffer(100)]; this.squared(); } @@ -32,6 +38,7 @@ describe('web worker transfer', () => { const transferables = []; const deserialized = deserialize(serialize(serializableMock, transferables)) as SerializableMock; expect(deserialize(serialize(serializableMock, transferables)) instanceof SerializableMock).toBeTruthy(); + expect(serializableMock.dataView instanceof DataView).toBeTruthy(); expect(serializableMock !== deserialized).toBeTruthy(); expect(deserialized.constructor === SerializableMock).toBeTruthy(); @@ -39,8 +46,11 @@ describe('web worker transfer', () => { expect(deserialized.buffer === serializableMock.buffer).toBeTruthy(); expect(deserialized.blob === serializableMock.blob).toBeTruthy(); expect(transferables[0] === serializableMock.buffer).toBeTruthy(); + expect(transferables[1] === serializableMock.dataView.buffer).toBeTruthy(); expect(deserialized._cached === undefined).toBeTruthy(); expect(deserialized.squared() === 100).toBeTruthy(); + expect(deserialized.dataView instanceof DataView).toBeTruthy(); + expect(deserialized.array).toEqual(serializableMock.array); }); test('anonymous class', () => { @@ -96,4 +106,23 @@ describe('web worker transfer', () => { expect(deserialized.statusText).toBe(statusText); expect(deserialized.url).toBe(url); }); + + test('serialize Object has $name', () => { + class BadClass { + _classRegistryKey: 'foo'; + } + const trySerialize = () => { + serialize(new BadClass()); + }; + expect(trySerialize).toThrow(); + }); + test('deserialize Object has $name', () => { + class BadClass { + _classRegistryKey: 'foo'; + } + const tryDeserialize = () => { + deserialize(new BadClass()); + }; + expect(tryDeserialize).toThrow(); + }); }); From ca635c0dca947e02117766c0ac5a3768abdc2a70 Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Wed, 5 Jun 2024 14:28:57 +0200 Subject: [PATCH 14/16] deserialize uses $name --- src/util/web_worker_transfer.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/web_worker_transfer.test.ts b/src/util/web_worker_transfer.test.ts index 81f59fd746..b9d3a20091 100644 --- a/src/util/web_worker_transfer.test.ts +++ b/src/util/web_worker_transfer.test.ts @@ -107,7 +107,7 @@ describe('web worker transfer', () => { expect(deserialized.url).toBe(url); }); - test('serialize Object has $name', () => { + test('serialize Object has _classRegistryKey', () => { class BadClass { _classRegistryKey: 'foo'; } @@ -117,11 +117,11 @@ describe('web worker transfer', () => { expect(trySerialize).toThrow(); }); test('deserialize Object has $name', () => { - class BadClass { - _classRegistryKey: 'foo'; - } + const badObject = { + '$name': 'foo' + }; const tryDeserialize = () => { - deserialize(new BadClass()); + deserialize(badObject); }; expect(tryDeserialize).toThrow(); }); From d9cf377bfc252b9f646b308087751a3026415073 Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Tue, 18 Jun 2024 09:48:42 +0200 Subject: [PATCH 15/16] add more tests - note how the test agains BigInt really shows that the implementation is missing the ability to serialize/deserialize some valid objects which can probably be seralized/deserialized by primitives --- src/util/web_worker_transfer.test.ts | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/util/web_worker_transfer.test.ts b/src/util/web_worker_transfer.test.ts index b9d3a20091..4969e0dc97 100644 --- a/src/util/web_worker_transfer.test.ts +++ b/src/util/web_worker_transfer.test.ts @@ -116,6 +116,20 @@ describe('web worker transfer', () => { }; expect(trySerialize).toThrow(); }); + test('serialize can not used reserved property #name', () => { + class BadClass { + static serialize() { + return { + '$name': 'foo' + }; + } + } + register('BadClass', BadClass); + const badObject = new BadClass(); + expect(() => { + serialize(badObject); + }).toThrow(); + }); test('deserialize Object has $name', () => { const badObject = { '$name': 'foo' @@ -125,4 +139,15 @@ describe('web worker transfer', () => { }; expect(tryDeserialize).toThrow(); }); + + test('some objects can not be serialized', () => { + expect(() => { + serialize(BigInt(123)); + }).toThrow(); + }); + test('some objects can not be deserialized', () => { + expect(() => { + deserialize(BigInt(123)); + }).toThrow(); + }); }); From 518ab096c119fdac71f26bb825572a42fcf7abcc Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Wed, 19 Jun 2024 14:17:22 +0200 Subject: [PATCH 16/16] explicit types --- src/util/web_worker_transfer.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/web_worker_transfer.ts b/src/util/web_worker_transfer.ts index 8cded07324..a8f1cd4325 100644 --- a/src/util/web_worker_transfer.ts +++ b/src/util/web_worker_transfer.ts @@ -92,12 +92,12 @@ function isArrayBuffer(value: any): value is ArrayBuffer { (value instanceof ArrayBuffer || (value.constructor && value.constructor.name === 'ArrayBuffer')); } -function getClassRegistryKey(input: Object|SerializedObject) { +function getClassRegistryKey(input: Object|SerializedObject): string { const klass = (input.constructor as any); return (input as SerializedObject).$name || klass._classRegistryKey; } -function isRegistered(input: unknown) { +function isRegistered(input: unknown): boolean { if (input === null || typeof input !== 'object') { return false; }