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

feat: switch from clone to built in structured clone #9068

Merged
merged 4 commits into from
Aug 31, 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
13 changes: 5 additions & 8 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
"@rollup/plugin-terser": "^0.4.3",
"@types/chai": "^4.3.5",
"@types/d3": "^7.4.0",
"@types/jest": "^27.4.1",
"@types/jest": "^29.5.4",
"@types/pako": "^2.0.0",
"@typescript-eslint/eslint-plugin": "^6.4.1",
"@typescript-eslint/parser": "^6.4.1",
Expand All @@ -107,9 +107,10 @@
"eslint-config-prettier": "^9.0.0",
"eslint-plugin-jest": "^27.2.3",
"eslint-plugin-prettier": "^5.0.0",
"fast-json-stable-stringify": "~2.1.0",
"highlight.js": "^11.8.0",
"jest": "^27.5.1",
"jest-dev-server": "^6.1.1",
"jest": "^29.6.3",
"jest-dev-server": "^9.0.0",
"mkdirp": "^3.0.1",
"pako": "^2.1.0",
"prettier": "^3.0.2",
Expand All @@ -130,10 +131,6 @@
"yaml-front-matter": "^4.1.1"
},
"dependencies": {
"@types/clone": "~2.1.1",
"clone": "~2.1.2",
"fast-deep-equal": "~3.1.3",
"fast-json-stable-stringify": "~2.1.0",
"json-stringify-pretty-compact": "~3.0.0",
"tslib": "~2.6.2",
"vega-event-selector": "~3.0.1",
Expand All @@ -145,6 +142,6 @@
"vega": "^5.24.0"
},
"engines": {
"node": ">=16"
"node": ">=18"
}
}
2 changes: 0 additions & 2 deletions rollup.config.mjs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import alias from '@rollup/plugin-alias';
import babel from '@rollup/plugin-babel';
import commonjs from '@rollup/plugin-commonjs';
import json from '@rollup/plugin-json';
import resolve from '@rollup/plugin-node-resolve';
import terser from '@rollup/plugin-terser';
Expand Down Expand Up @@ -67,7 +66,6 @@ const outputs = [
}
}),
resolve({browser: true, extensions}),
commonjs(),
json(),
babel({
extensions,
Expand Down
135 changes: 122 additions & 13 deletions src/util.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
import {default as clone_} from 'clone';
import deepEqual_ from 'fast-deep-equal';
import stableStringify from 'fast-json-stable-stringify';
import {hasOwnProperty, isNumber, isString, splitAccessPath, stringValue, writeConfig} from 'vega-util';
import {isLogicalAnd, isLogicalNot, isLogicalOr, LogicalComposition} from './logical';

export const deepEqual = deepEqual_;
export const duplicate = clone_;
export const duplicate = structuredClone;

export function never(message: string): never {
throw new Error(message);
Expand Down Expand Up @@ -46,14 +42,9 @@
* Monkey patch Set so that `stringify` produces a string representation of sets.
*/
Set.prototype['toJSON'] = function () {
return `Set(${[...this].map(x => stableStringify(x)).join(',')})`;
return `Set(${[...this].map(x => stringify(x)).join(',')})`;
};

/**
* Converts any object to a string representation that can be consumed by humans.
*/
export const stringify = stableStringify;

/**
* Converts any object to a string of limited size, or a number.
*/
Expand All @@ -62,7 +53,7 @@
return a;
}

const str = isString(a) ? a : stableStringify(a);
const str = isString(a) ? a : stringify(a);

// short strings can be used as hash directly, longer strings are hashed to reduce memory usage
if (str.length < 250) {
Expand Down Expand Up @@ -152,7 +143,7 @@
export type Dict<T> = Record<string, T>;

/**
* Returns true if the two dictionaries disagree. Applies only to defined values.
* Returns true if the two dictionaries agree. Applies only to defined values.
*/
export function isEqual<T>(dict: Dict<T>, other: Dict<T>) {
const dictKeys = keys(dict);
Expand Down Expand Up @@ -400,3 +391,121 @@
}
return !isNaN(value as any) && !isNaN(parseFloat(value));
}

const clonedProto = Object.getPrototypeOf(structuredClone({}));

/**
* Compares two values for equality, including arrays and objects.
*
* Adapted from https://github.com/epoberezkin/fast-deep-equal.
*/
export function deepEqual(a: any, b: any) {
if (a === b) return true;

if (a && b && typeof a == 'object' && typeof b == 'object') {
// compare names to avoid issues with structured clone
if (a.constructor.name !== b.constructor.name) return false;

let length;
let i;

if (Array.isArray(a)) {
length = a.length;
if (length != b.length) return false;
for (i = length; i-- !== 0; ) if (!deepEqual(a[i], b[i])) return false;
return true;
}

if (a instanceof Map && b instanceof Map) {
if (a.size !== b.size) return false;
for (i of a.entries()) if (!b.has(i[0])) return false;
for (i of a.entries()) if (!deepEqual(i[1], b.get(i[0]))) return false;
return true;

Check warning on line 423 in src/util.ts

View check run for this annotation

Codecov / codecov/patch

src/util.ts#L423

Added line #L423 was not covered by tests
}

if (a instanceof Set && b instanceof Set) {
if (a.size !== b.size) return false;
for (i of a.entries()) if (!b.has(i[0])) return false;
return true;

Check warning on line 429 in src/util.ts

View check run for this annotation

Codecov / codecov/patch

src/util.ts#L429

Added line #L429 was not covered by tests
}

if (ArrayBuffer.isView(a) && ArrayBuffer.isView(b)) {
length = (a as any).length;

Check warning on line 433 in src/util.ts

View check run for this annotation

Codecov / codecov/patch

src/util.ts#L433

Added line #L433 was not covered by tests
if (length != (b as any).length) return false;
for (i = length; i-- !== 0; ) if (a[i] !== b[i]) return false;
return true;

Check warning on line 436 in src/util.ts

View check run for this annotation

Codecov / codecov/patch

src/util.ts#L436

Added line #L436 was not covered by tests
}

if (a.constructor === RegExp) return a.source === b.source && a.flags === b.flags;
// also compare to structured clone prototype
if (a.valueOf !== Object.prototype.valueOf && a.valueOf !== clonedProto.valueOf) return a.valueOf() === b.valueOf();
if (a.toString !== Object.prototype.toString && a.toString !== clonedProto.toString)
return a.toString() === b.toString();

Check warning on line 443 in src/util.ts

View check run for this annotation

Codecov / codecov/patch

src/util.ts#L443

Added line #L443 was not covered by tests

const ks = Object.keys(a);
length = ks.length;
if (length !== Object.keys(b).length) return false;

for (i = length; i-- !== 0; ) if (!Object.prototype.hasOwnProperty.call(b, ks[i])) return false;

for (i = length; i-- !== 0; ) {
const key = ks[i];

if (!deepEqual(a[key], b[key])) return false;
}

return true;
}

// true if both NaN, false otherwise
return a !== a && b !== b;
}

/**
* Converts any object to a string representation that can be consumed by humans.
*
* Adapted from https://github.com/epoberezkin/fast-json-stable-stringify
*/
export function stringify(data: any) {
const seen: any[] = [];

return (function _stringify(node: any) {
if (node && node.toJSON && typeof node.toJSON === 'function') {
node = node.toJSON();
}

if (node === undefined) return undefined;
if (typeof node == 'number') return isFinite(node) ? '' + node : 'null';
if (typeof node !== 'object') return JSON.stringify(node);

let i, out;
if (Array.isArray(node)) {
out = '[';
for (i = 0; i < node.length; i++) {
if (i) out += ',';
out += _stringify(node[i]) || 'null';
}
return out + ']';
}

if (node === null) return 'null';

if (seen.includes(node)) {
throw new TypeError('Converting circular structure to JSON');

Check warning on line 494 in src/util.ts

View check run for this annotation

Codecov / codecov/patch

src/util.ts#L494

Added line #L494 was not covered by tests
}

const seenIndex = seen.push(node) - 1;
const ks = Object.keys(node).sort();
out = '';
for (i = 0; i < ks.length; i++) {
const key = ks[i];
const value = _stringify(node[key]);

if (!value) continue;
if (out) out += ',';
out += JSON.stringify(key) + ':' + value;
}
seen.splice(seenIndex, 1);
return `{${out}}`;
})(data);
}
15 changes: 15 additions & 0 deletions test-runtime/babel.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Only used for jest
module.exports = {
presets: [
[
'@babel/preset-env',
{
targets: {
node: 'current'
}
}
],
'@babel/preset-typescript'
],
plugins: ['@babel/proposal-class-properties']
};
2 changes: 1 addition & 1 deletion test-runtime/discrete.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ describe(`point selections at runtime in unit views`, () => {
let testRender: (filename: string) => Promise<void>;

beforeAll(async () => {
page = await (global as any).__BROWSER__.newPage();
page = await (global as any).__BROWSER_GLOBAL__.newPage();
embed = embedFn(page);
testRender = testRenderFn(page, `${type}/unit`);
await page.goto('http://0.0.0.0:8000/test-runtime/');
Expand Down
2 changes: 1 addition & 1 deletion test-runtime/interval.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ describe('interval selections at runtime in unit views', () => {
let testRender: (filename: string) => Promise<void>;

beforeAll(async () => {
page = await (global as any).__BROWSER__.newPage();
page = await (global as any).__BROWSER_GLOBAL__.newPage();
embed = embedFn(page);
testRender = testRenderFn(page, `${type}/unit`);
await page.goto('http://0.0.0.0:8000/test-runtime/');
Expand Down
15 changes: 9 additions & 6 deletions test-runtime/puppeteer_environment.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
const fs = require('fs');
const {readFile} = require('fs').promises;
const os = require('os');
const path = require('path');
const puppeteer = require('puppeteer');
const NodeEnvironment = require('jest-environment-node');
const NodeEnvironment = require('jest-environment-node').TestEnvironment;

const DIR = path.join(os.tmpdir(), 'jest_puppeteer_global_setup');

Expand All @@ -14,23 +14,26 @@ class PuppeteerEnvironment extends NodeEnvironment {
async setup() {
await super.setup();
// get the wsEndpoint
const wsEndpoint = fs.readFileSync(path.join(DIR, 'wsEndpoint'), 'utf8');
const wsEndpoint = await readFile(path.join(DIR, 'wsEndpoint'), 'utf8');
if (!wsEndpoint) {
throw new Error('wsEndpoint not found');
}

// connect to puppeteer
this.global.__BROWSER__ = await puppeteer.connect({
this.global.__BROWSER_GLOBAL__ = await puppeteer.connect({
browserWSEndpoint: wsEndpoint
});
}

async teardown() {
if (this.global.__BROWSER_GLOBAL__) {
this.global.__BROWSER_GLOBAL__.disconnect();
}
await super.teardown();
}

runScript(script) {
return super.runScript(script);
getVmContext() {
return super.getVmContext();
}
}

Expand Down
2 changes: 1 addition & 1 deletion test-runtime/resolve.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ for (const type of selectionTypes) {
let embed: (specification: TopLevelSpec) => Promise<void>;

beforeAll(async () => {
page = await (global as any).__BROWSER__.newPage();
page = await (global as any).__BROWSER_GLOBAL__.newPage();
embed = embedFn(page);
await page.goto('http://0.0.0.0:8000/test-runtime/');
});
Expand Down
11 changes: 5 additions & 6 deletions test-runtime/setup.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
const fs = require('fs');
const {mkdir, writeFile} = require('fs').promises;
const os = require('os');
const path = require('path');
const mkdirp = require('mkdirp');
const puppeteer = require('puppeteer');
const {setup: setupDevServer} = require('jest-dev-server');
const chalk = require('chalk');

const DIR = path.join(os.tmpdir(), 'jest_puppeteer_global_setup');

module.exports = async function () {
await setupDevServer({
globalThis.servers = await setupDevServer({
command: `node ./node_modules/.bin/serve -l 8000`,
launchTimeout: 50000,
port: 8000
Expand All @@ -20,9 +19,9 @@ module.exports = async function () {
const browser = await puppeteer.launch();
// store the browser instance so we can teardown it later
// this global is only available in the teardown but not in TestEnvironments
global.__BROWSER_GLOBAL__ = browser;
globalThis.__BROWSER_GLOBAL__ = browser;

// use the file system to expose the wsEndpoint for TestEnvironments
mkdirp.sync(DIR);
fs.writeFileSync(path.join(DIR, 'wsEndpoint'), browser.wsEndpoint());
await mkdir(DIR, {recursive: true});
await writeFile(path.join(DIR, 'wsEndpoint'), browser.wsEndpoint());
};
9 changes: 5 additions & 4 deletions test-runtime/teardown.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
const fs = require('fs').promises;
const os = require('os');
const path = require('path');
const rimraf = require('rimraf');
const {teardown: teardownDevServer} = require('jest-dev-server');

const DIR = path.join(os.tmpdir(), 'jest_puppeteer_global_setup');

module.exports = async function () {
await teardownDevServer();
await teardownDevServer(globalThis.servers);

// close the browser instance
await global.__BROWSER_GLOBAL__.close();
await globalThis.__BROWSER_GLOBAL__.close();

// clean-up the wsEndpoint file
rimraf.sync(DIR);
await fs.rm(DIR, {recursive: true, force: true});
};
2 changes: 1 addition & 1 deletion test-runtime/toggle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe('Toggle point selections at runtime', () => {
let testRender: (filename: string) => Promise<void>;

beforeAll(async () => {
page = await (global as any).__BROWSER__.newPage();
page = await (global as any).__BROWSER_GLOBAL__.newPage();
embed = embedFn(page);
testRender = testRenderFn(page, 'point/toggle');
await page.goto('http://0.0.0.0:8000/test-runtime/');
Expand Down
2 changes: 1 addition & 1 deletion test-runtime/translate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe('Translate interval selections at runtime', () => {
let testRender: (filename: string) => Promise<void>;

beforeAll(async () => {
page = await (global as any).__BROWSER__.newPage();
page = await (global as any).__BROWSER_GLOBAL__.newPage();
embed = embedFn(page);
await page.goto('http://0.0.0.0:8000/test-runtime/');
});
Expand Down
2 changes: 1 addition & 1 deletion test-runtime/zoom.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('Zoom interval selections at runtime', () => {
let testRender: (filename: string) => Promise<void>;

beforeAll(async () => {
page = await (global as any).__BROWSER__.newPage();
page = await (global as any).__BROWSER_GLOBAL__.newPage();
embed = embedFn(page);
await page.goto('http://0.0.0.0:8000/test-runtime/');
});
Expand Down
Loading