Skip to content

Commit

Permalink
[fei4960.2.jsdom] Migrate JSDOM render environment (#607)
Browse files Browse the repository at this point in the history
## Summary:
This migrates the JSDOM render environment from Khan/render-gateway. Now it is its own package as it should've been.

This also tweaks the rollup config to cope with dynamic imports which was needed for the new package to build.

The only migration piece left is to bring in the example server implementations for testing various paths through the code.

Issue: FEI-4960

## Test plan:
`yarn test`
`yarn build`
`yarn typecheck`

Author: somewhatabstract

Reviewers: kevinbarabash, somewhatabstract, github-code-scanning[bot]

Required Reviewers:

Approved By: kevinbarabash

Checks: ⌛ Test (macos-latest, 16.x), ✅ codecov/project, ✅ CodeQL, ✅ Lint, typecheck, and coverage check (ubuntu-latest, 16.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 16.x), ✅ gerald, ✅ Analyze (javascript), ⏭  dependabot

Pull Request URL: #607
  • Loading branch information
somewhatabstract authored Mar 31, 2023
1 parent b20ecde commit 579e618
Show file tree
Hide file tree
Showing 29 changed files with 4,078 additions and 13 deletions.
5 changes: 5 additions & 0 deletions .changeset/healthy-ladybugs-travel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-stuff-render-server": patch
---

Modified ICloseable to use void
5 changes: 5 additions & 0 deletions .changeset/serious-frogs-lay.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-stuff-render-environment-jsdom": major
---

Migrated JSDOM render environment from Khan/render-gateway
25 changes: 19 additions & 6 deletions build-settings/rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,21 @@ const makePackageBasedPath = (pkgName, pkgRelPath) =>
/**
* Generate the rollup output configuration for a given
*/
const createOutputConfig = (pkgName, format, targetFile) => ({
file: makePackageBasedPath(pkgName, targetFile),
sourcemap: true,
format,
});
const createOutputConfig = (pkgName, format, targetFile, isSingleFile) => {
const outputConfig = {
sourcemap: true,
format,
};
if (isSingleFile) {
outputConfig.file = makePackageBasedPath(pkgName, targetFile);
} else {
outputConfig.dir = makePackageBasedPath(
pkgName,
path.dirname(targetFile),
);
}
return outputConfig;
};

/**
* Get a set of strings from a given string, returning the defaults
Expand Down Expand Up @@ -103,8 +113,9 @@ const createConfig = (

const extensions = [".js", ".jsx", ".ts", ".tsx"];

const isSingleFile = inputFile != null;
const config = {
output: createOutputConfig(name, format, file),
output: createOutputConfig(name, format, file, isSingleFile),
input: makePackageBasedPath(name, inputFile || "./src/index.ts"),
plugins: [
// We don't want to do process.env.NODE_ENV checks in our main
Expand Down Expand Up @@ -181,6 +192,7 @@ const getPackageInfo = (commandLineArgs, pkgName) => {
platform: "browser",
file: cjsBrowser,
plugins: [],
inputFile: `./src/index.ts`,
});
}
if (formats.has("esm") && esmBrowser) {
Expand All @@ -191,6 +203,7 @@ const getPackageInfo = (commandLineArgs, pkgName) => {
file: esmBrowser,
// We care about the file size of this one.
plugins: [filesize()],
inputFile: `./src/index.ts`,
});
}
}
Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"@types/express-winston": "^4.0.0",
"@types/jest": "^29.5.0",
"@types/jest-when": "^3.5.2",
"@types/jsdom": "^21.1.1",
"@rollup/plugin-terser": "^0.4.0",
"@types/superagent": "^4.1.16",
"@types/winston": "^2.4.4",
Expand Down Expand Up @@ -60,6 +61,7 @@
"jest": "^29.5.0",
"jest-extended": "^3.2.4",
"jest-when": "^3.5.2",
"jsdom": "^21.1.1",
"npm-package-json-lint": "^6.4.0",
"prettier": "^2.8.7",
"rollup": "^2.79.1",
Expand Down
9 changes: 8 additions & 1 deletion packages/wonder-stuff-render-environment-jsdom/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,17 @@
"scripts": {
"test": "bash -c 'yarn --silent --cwd \"../..\" test ${@:0} $($([[ ${@: -1} = -* ]] || [[ ${@: -1} = bash ]]) && echo $PWD)'"
},
"dependencies": {},
"dependencies": {
"@khanacademy/wonder-stuff-core": "^1.3.0",
"@khanacademy/wonder-stuff-server": "^4.0.2",
"@khanacademy/wonder-stuff-render-server": "^0.0.1"
},
"devDependencies": {
"ws-dev-build-settings": "^1.0.0"
},
"peerDependencies": {
"jsdom": "^21.1.1"
},
"author": "",
"license": "MIT"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
THIS IS TEST CONTENT FOR THE SNAPSHOT!
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`JSDOMConfiguration #constructor should throw if invalid getFileList is provided 1`] = `"Must provide valid callback for obtaining file list"`;

exports[`JSDOMConfiguration #constructor should throw if invalid getFileList is provided 2`] = `"Must provide valid callback for obtaining file list"`;

exports[`JSDOMConfiguration #constructor should throw if invalid getResourceLoader is provided 1`] = `"Must provide valid callback for obtaining resource loader"`;

exports[`JSDOMConfiguration #constructor should throw if invalid getResourceLoader is provided 2`] = `"Must provide valid callback for obtaining resource loader"`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`JSDOMEnvironment #render should reject if result is malformed ({ body: 'NEED MORE THAN THIS' }) 1`] = `"Malformed render result: {"body":"NEED MORE THAN THIS"}"`;

exports[`JSDOMEnvironment #render should reject if result is malformed ({ body: 'THIS HELPS BUT WHERE ARE THE HEADERS', status: 200 }) 1`] = `"Malformed render result: {"body":"THIS HELPS BUT WHERE ARE THE HEADERS","status":200}"`;

exports[`JSDOMEnvironment #render should reject if result is malformed ({ status: 200, headers: {} }) 1`] = `"Malformed render result: {"status":200,"headers":{}}"`;

exports[`JSDOMEnvironment #render should reject if result is malformed (THIS IS NOT CORRECT) 1`] = `"Malformed render result: "THIS IS NOT CORRECT""`;

exports[`JSDOMEnvironment #render should reject if result is malformed (null) 1`] = `"Malformed render result: null"`;

exports[`JSDOMEnvironment #render should reject if result is malformed (undefined) 1`] = `"Malformed render result: undefined"`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import {applyAbortablePromisesPatch} from "../apply-abortable-promises-patch";

describe("#applyAbortablePromisesPatch", () => {
afterEach(() => {
// @ts-expect-error We know promise doesn't usually have this
delete Promise.prototype.abort;
});

it("should add an abort method to the promise prototype", () => {
// Arrange

// Act
applyAbortablePromisesPatch();
const result: any = Promise.resolve();

// Assert
expect(result.abort).toBeFunction();
});

it("should replace any existing abort method on the promise prototype", () => {
// Arrange
// @ts-expect-error We know promise doesn't usually have this
Promise.prototype.abort = "ABORT_FN";

// Act
applyAbortablePromisesPatch();
const result: any = Promise.resolve();

// Assert
expect(result.abort).toBeFunction();
});

it("should not delete the existing function if it was applied by us", () => {
// Arrange
applyAbortablePromisesPatch();
// @ts-expect-error We know promise doesn't usually have this
const abortFn = Promise.prototype.abort;

// Act
applyAbortablePromisesPatch();
const result: any = Promise.resolve();

// Assert
expect(result.abort).toBe(abortFn);
});

it("should delete the existing function if it was applied by us and force is true", () => {
// Arrange
applyAbortablePromisesPatch();
// @ts-expect-error We know promise doesn't usually have this
const abortFn = Promise.prototype.abort;

// Act
applyAbortablePromisesPatch(true);
const result: any = Promise.resolve();

// Assert
expect(result.abort).not.toBe(abortFn);
expect(result.abort).toBeFunction();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
import * as WSRenderServer from "@khanacademy/wonder-stuff-render-server";
import {CloseableVirtualConsole} from "../closeable-virtual-console";

jest.mock("@khanacademy/wonder-stuff-render-server");

describe("CloseableVirtualConsole", () => {
describe("before close() is called", () => {
it("should ignore jsdomError events for 'Could not load img'", () => {
// Arrange
const fakeLogger: any = {
error: jest.fn(),
};
const underTest = new CloseableVirtualConsole(fakeLogger);

// Act
underTest.emit("jsdomError", new Error("Could not load img"));

// Assert
expect(fakeLogger.error).not.toHaveBeenCalled();
});

it("should report jsdomError events as logger.error", () => {
// Arrange
const fakeLogger: any = {
error: jest.fn(),
};
jest.spyOn(WSRenderServer, "extractError").mockReturnValue({
error: "ERROR_MESSAGE",
stack: "ERROR_STACK",
});
const underTest = new CloseableVirtualConsole(fakeLogger);

// Act
underTest.emit(
"jsdomError",
new Error("This is a jsdomError message"),
);

// Assert
expect(fakeLogger.error).toHaveBeenCalledWith(
"JSDOM jsdomError:ERROR_MESSAGE",
{
error: "ERROR_MESSAGE",
kind: "Internal",
stack: "ERROR_STACK",
},
);
});

it("should pass errors to logger.error with args as metadata", () => {
// Arrange
const fakeLogger: any = {
error: jest.fn(),
};
const underTest = new CloseableVirtualConsole(fakeLogger);

// Act
underTest.emit(
"error",
"This is an error message",
"and these are args",
);

// Assert
expect(fakeLogger.error).toHaveBeenCalledWith(
"JSDOM error:This is an error message",
{
args: ["and these are args"],
},
);
});

it.each(["warn", "info", "log", "debug"])(
"should pass %s through to logger silly with args as metadata",
(method: string) => {
// Arrange
const fakeLogger: any = {
silly: jest.fn(),
};
const underTest = new CloseableVirtualConsole(fakeLogger);

// Act
underTest.emit(
method,
"This is a logged message",
"and these are args",
);

// Assert
expect(fakeLogger.silly).toHaveBeenCalledWith(
`JSDOM ${method}:This is a logged message`,
{
args: ["and these are args"],
},
);
},
);
});

describe("after close() is called", () => {
it.each(["jsdomError", "error", "warn", "log", "debug", "info"])(
"it should not log anything for %s",
(method: any) => {
// Arrange
const fakeLogger: any = {
/*nothing*/
};
const console = new CloseableVirtualConsole(fakeLogger);

// Act
console.close();
const underTest = () =>
console.emit(
method,
"This is a logged message",
"and these are args",
);

// Assert
expect(underTest).not.toThrow();
},
);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
describe("index.js", () => {
it("should export what we expect it to export", async () => {
// Arrange
const importedModule = import("../index");

// Act
const result = await importedModule;

// Assert
expect(result).toContainAllKeys([
"Configuration",
"Environment",
"ResourceLoader",
"FileResourceLoader",
]);
});
});
Loading

0 comments on commit 579e618

Please sign in to comment.