Skip to content

Commit

Permalink
fix(client-utils): fix trace.js import of performance (#21257) (#21322)
Browse files Browse the repository at this point in the history
Fix GH #21252

Cherry pick of
925a477

Co-authored-by: Daniel Lehenbauer <[email protected]>
Co-authored-by: Jason Hartman <[email protected]>
  • Loading branch information
3 people authored Jun 5, 2024
1 parent b84f9bc commit d02a5e9
Show file tree
Hide file tree
Showing 14 changed files with 182 additions and 43 deletions.
23 changes: 23 additions & 0 deletions packages/common/client-utils/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,29 @@ If you want to add code that does not meet these requirements, these other packa
**core-interfaces** package.
- **Zero-dependency shared code** should be put in the **core-utils** package.

## Isomorphic Code

One of the primary reasons for this package's existence is to provide isomorphic implementations of
Buffer and related utilities that work in both browser and Node.js environments.

Our general strategy for this is as follows:

- We use the export map in package.json to provide different entrypoints for browser (indexBrowser.js)
vs. Node.js (indexNode.js).

- Because the browser ecosystem is more complex (bunders, etc.), we improve our odds of success by making
the browser the default. Only Node.js relies on remapping via the export map.

- We further simplify things by only using the export map to resolve the initial entrypoint. We do not
rely on export maps to remap imports within the module. (Basically, the browser / node.js specific
implementations fork at the entrypoint and from that point on explicitly import browser or node
specific files.)

One thing it is important to be aware of is that our CJS support relies on copying a stub package.json
file to dist/package.json to set the module type to commonjs. When resolving internal imports for CJS
packages, module resolution will walk up from the \*.js file and discover this stub package.json. Because
the stub package.json lacks an export map, internal imports will not be remapped.

<!-- AUTO-GENERATED-CONTENT:START (README_DEPENDENCY_GUIDELINES_SECTION:includeHeading=TRUE) -->

<!-- prettier-ignore-start -->
Expand Down
28 changes: 3 additions & 25 deletions packages/common/client-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,32 +34,10 @@
"default": "./dist/indexBrowser.js"
}
}
},
"./bufferBrowser.js": {
"node": {
"import": {
"types": "./lib/bufferNode.d.ts",
"default": "./lib/bufferNode.js"
},
"require": {
"types": "./dist/bufferNode.d.ts",
"default": "./dist/bufferNode.js"
}
},
"default": {
"import": {
"types": "./lib/bufferBrowser.d.ts",
"default": "./lib/bufferBrowser.js"
},
"require": {
"types": "./dist/bufferBrowser.d.ts",
"default": "./dist/bufferBrowser.js"
}
}
}
},
"main": "lib/indexNode.js",
"types": "lib/indexNode.d.ts",
"main": "lib/indexBrowser.js",
"types": "lib/indexBrowser.d.ts",
"scripts": {
"build": "fluid-build . --task build",
"build:commonjs": "fluid-build . --task commonjs",
Expand Down Expand Up @@ -89,7 +67,7 @@
"test:mocha": "npm run test:mocha:esm && npm run test:mocha:cjs",
"test:mocha:cjs": "mocha --recursive \"dist/test/mocha/**/*.spec.*js\" --exit -r node_modules/@fluid-internal/mocha-test-setup",
"test:mocha:esm": "mocha --recursive \"lib/test/mocha/**/*.spec.*js\" --exit -r node_modules/@fluid-internal/mocha-test-setup",
"tsc": "fluid-tsc commonjs --project ./tsconfig.cjs.json && copyfiles -f ./src/cjs/package.json ./dist",
"tsc": "fluid-tsc commonjs --project ./tsconfig.cjs.json && copyfiles -f ../../../common/build/build-common/src/cjs/package.json ./dist",
"typetests:gen": "fluid-type-test-generator",
"typetests:prepare": "flub typetests --dir . --reset --previous --normalize"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
* Licensed under the MIT License.
*/

// Note: The exports map in package.json will correct this import to "./bufferNode.js" in node environments
// This file is a Node.js-specific implementation of the base64 encoding functions.
// Aside from the below import statement, this file should be identical to the
// base64EncodingNode.ts.
//
// (See 'Isomorphic Code' section in the package README.md.)
import { IsoBuffer } from "./bufferBrowser.js";

/**
Expand Down
50 changes: 50 additions & 0 deletions packages/common/client-utils/src/base64EncodingNode.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

// This file is a Node.js-specific implementation of the base64 encoding functions.
// Aside from the below import statement, this file should be identical to the
// base64EncodingBrowser.ts.
//
// (See 'Isomorphic Code' section in the package README.md.)
import { IsoBuffer } from "./bufferNode.js";

/**
* Converts the provided {@link https://en.wikipedia.org/wiki/Base64 | base64}-encoded string
* to {@link https://en.wikipedia.org/wiki/UTF-8 | utf-8}.
*
* @internal
*/
export const fromBase64ToUtf8 = (input: string): string =>
IsoBuffer.from(input, "base64").toString("utf8");

/**
* Converts the provided {@link https://en.wikipedia.org/wiki/UTF-8 | utf-8}-encoded string
* to {@link https://en.wikipedia.org/wiki/Base64 | base64}.
*
* @internal
*/
export const fromUtf8ToBase64 = (input: string): string =>
IsoBuffer.from(input, "utf8").toString("base64");

/**
* Convenience function to convert unknown encoding to utf8 that avoids
* buffer copies/encode ops when no conversion is needed.
* @param input - The source string to convert.
* @param encoding - The source string's encoding.
*
* @internal
*/
export const toUtf8 = (input: string, encoding: string): string => {
switch (encoding) {
case "utf8":
// eslint-disable-next-line unicorn/text-encoding-identifier-case -- this value is supported, just discouraged
case "utf-8": {
return input;
}
default: {
return IsoBuffer.from(input, encoding).toString();
}
}
};
7 changes: 0 additions & 7 deletions packages/common/client-utils/src/cjs/package.json

This file was deleted.

1 change: 1 addition & 0 deletions packages/common/client-utils/src/hashFileBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import * as base64js from "base64-js";

// Note: See 'Isomorphic Code' section in the package README.md
import { IsoBuffer } from "./bufferBrowser.js";

async function digestBuffer(file: IsoBuffer, algorithm: "SHA-1" | "SHA-256"): Promise<Uint8Array> {
Expand Down
1 change: 1 addition & 0 deletions packages/common/client-utils/src/hashFileNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import { sha1, sha256 } from "sha.js";

// Note: See 'Isomorphic Code' section in the package README.md
import type { IsoBuffer } from "./bufferNode.js";

/**
Expand Down
5 changes: 4 additions & 1 deletion packages/common/client-utils/src/indexBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
* Licensed under the MIT License.
*/

// Entrypoint for browser-specific code in the package.
// (See 'Isomorphic Code' section in the package README.md.)

export {
bufferToString,
isArrayBuffer,
Expand All @@ -13,7 +16,7 @@ export {
export { gitHashFile, hashFile } from "./hashFileBrowser.js";
export { performance } from "./performanceIsomorphic.js";

export { fromBase64ToUtf8, fromUtf8ToBase64, toUtf8 } from "./base64Encoding.js";
export { fromBase64ToUtf8, fromUtf8ToBase64, toUtf8 } from "./base64EncodingBrowser.js";
export { Uint8ArrayToArrayBuffer } from "./bufferShared.js";
export { EventEmitter } from "./eventEmitter.cjs";
export { type IsomorphicPerformance } from "./performanceIsomorphic.js";
Expand Down
5 changes: 4 additions & 1 deletion packages/common/client-utils/src/indexNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@
* Licensed under the MIT License.
*/

// Entrypoint for Node.js-specific code in the package.
// (See 'Isomorphic Code' section in the package README.md.)

export { type Buffer } from "./bufferNode.js";
export { bufferToString, IsoBuffer, stringToBuffer, Uint8ArrayToString } from "./bufferNode.js";
export { gitHashFile, hashFile } from "./hashFileNode.js";
export { performance } from "./performanceIsomorphic.js";

export { fromBase64ToUtf8, fromUtf8ToBase64, toUtf8 } from "./base64Encoding.js";
export { fromBase64ToUtf8, fromUtf8ToBase64, toUtf8 } from "./base64EncodingNode.js";
export { Uint8ArrayToArrayBuffer } from "./bufferShared.js";
export { EventEmitter } from "./eventEmitter.cjs";
export { IsomorphicPerformance } from "./performanceIsomorphic.js";
Expand Down
24 changes: 17 additions & 7 deletions packages/common/client-utils/src/test/jest/buffer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ import * as BufferBrowser from "../../bufferBrowser.js";
import * as BufferNode from "../../bufferNode.js";

describe("Buffer isomorphism", () => {
test("returns the expected implementation", () => {
// BufferNode should create a native Node.js Buffer instance
const nodeBuffer = BufferNode.IsoBuffer.from("", "utf8");
expect(nodeBuffer.constructor.name).toEqual("Buffer");

// BufferBrowser should create our partial Buffer polyfil.
const browserBuffer = BufferBrowser.IsoBuffer.from("", "utf8");
expect(browserBuffer.constructor.name).toEqual("IsoBuffer");
});

test("from string utf-8/16 is compatible", () => {
const testArray = [
"",
Expand Down Expand Up @@ -135,7 +145,7 @@ describe("Buffer isomorphism", () => {

const encodedWithoutView = new TextEncoder().encode(item).buffer;
const nodeBufferWithoutView = BufferNode.IsoBuffer.from(encodedWithoutView);
const browserBufferWithoutView = BufferNode.IsoBuffer.from(encodedWithoutView);
const browserBufferWithoutView = BufferBrowser.IsoBuffer.from(encodedWithoutView);

expect(nodeBufferWithoutView.toString("base64")).toEqual(nodeBuffer.toString("base64"));
expect(browserBufferWithoutView.toString("base64")).toEqual(
Expand Down Expand Up @@ -253,14 +263,14 @@ describe("Buffer isomorphism", () => {
test("bufferToString working with IsoBuffer", () => {
const test = "aGVsbG90aGVyZQ==";

const buffer = BufferBrowser.IsoBuffer.from(test, "base64");
expect(BufferBrowser.bufferToString(buffer, "base64")).toEqual(test);
const browserBuffer = BufferBrowser.IsoBuffer.from(test, "base64");
expect(BufferBrowser.bufferToString(browserBuffer, "base64")).toEqual(test);
// eslint-disable-next-line unicorn/text-encoding-identifier-case -- this value is supported, just discouraged
expect(BufferBrowser.bufferToString(buffer, "utf-8")).toEqual("hellothere");
expect(BufferBrowser.bufferToString(browserBuffer, "utf-8")).toEqual("hellothere");

const buffer2 = BufferNode.IsoBuffer.from(test, "base64");
expect(BufferNode.bufferToString(buffer2, "base64")).toEqual(test);
const nodeBuffer = BufferNode.IsoBuffer.from(test, "base64");
expect(BufferNode.bufferToString(nodeBuffer, "base64")).toEqual(test);
// eslint-disable-next-line unicorn/text-encoding-identifier-case -- this value is supported, just discouraged
expect(BufferNode.bufferToString(buffer2, "utf-8")).toEqual("hellothere");
expect(BufferNode.bufferToString(nodeBuffer, "utf-8")).toEqual("hellothere");
});
});
17 changes: 17 additions & 0 deletions packages/common/client-utils/src/test/mocha/base64Encoding.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

import { strict as assert } from "node:assert";

import { fromBase64ToUtf8, fromUtf8ToBase64, toUtf8 } from "../../indexNode.js";

describe("base64Encoding", () => {
it("round-trips correctly", async () => {
const original = "hello world";
const base64 = fromUtf8ToBase64(original);
assert.equal(fromBase64ToUtf8(base64), original);
assert.equal(toUtf8(base64, "base64"), original);
});
});
22 changes: 22 additions & 0 deletions packages/common/client-utils/src/test/mocha/buffer.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

import { strict as assert } from "node:assert";

import { IsoBuffer } from "../../indexNode.js";

describe("IsoBuffer", () => {
it("is a Buffer", () => {
// Paranoid test that 'IsoBuffer' can be constructed in both CJS/ESM in Node.js environments.
// Note that no export remapping is involved, given that 'IsoBuffer' is imported from 'indexNode.js'.
//
// More comprehensive testing for 'IsoBuffer' is done in 'jest/buffer.spec.ts', which compares
// Node.js and Broswer implementations for equivalence.
assert(
IsoBuffer.from("", "utf8") instanceof Buffer,
"Expected IsoBuffer.from to return a native Node.js Buffer instance.",
);
});
});
34 changes: 34 additions & 0 deletions packages/common/client-utils/src/test/mocha/trace.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

import { strict as assert } from "node:assert";

import { Trace } from "../../indexNode.js";

describe("Trace", () => {
it("measure 2ms timeout", async () => {
const trace = Trace.start();

return new Promise((resolve, reject) => {
setTimeout(() => {
try {
const event = trace.trace();

// While we specify a 2ms timeout, it's possible for performance.now() to measure a
// slightly smaller duration due to timing attack and fingerprinting mitigations.
// (See https://developer.mozilla.org/en-US/docs/Web/API/Performance/now#security_requirements)
//
// Therefore, we conservatively require that only 1ms has elapsed.
assert(event.duration >= 1);
assert(event.totalTimeElapsed >= 1);

resolve(undefined);
} catch (error) {
reject(error);
}
}, 2);
});
});
});
2 changes: 1 addition & 1 deletion packages/common/client-utils/src/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Licensed under the MIT License.
*/

import { performance } from "./indexNode.js";
import { performance } from "./performanceIsomorphic.js";

/**
* Helper class for tracing performance of events
Expand Down

0 comments on commit d02a5e9

Please sign in to comment.