Skip to content

Refactor Hooks #295

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
8 changes: 7 additions & 1 deletion packages/engine/src/CellEvaluator.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { TypeCellContext } from "./context";
import { ModuleExecution, runModule } from "./executor";
import { HookExecution } from "./HookExecution";
import { createExecutionScope, getModulesFromTypeCellCode } from "./modules";
import { isReactView } from "./reactView";

Expand Down Expand Up @@ -49,7 +50,11 @@ export function createCellEvaluator(
onOutputChanged(error);
}

const executionScope = createExecutionScope(typecellContext);
const hookExecution = new HookExecution();
const executionScope = createExecutionScope(
typecellContext,
hookExecution.context
);
let moduleExecution: ModuleExecution | undefined;

async function evaluate(compiledCode: string) {
Expand All @@ -69,6 +74,7 @@ export function createCellEvaluator(
modules[0],
typecellContext,
resolveImport,
hookExecution,
beforeExecuting,
onExecuted,
onError,
Expand Down
97 changes: 97 additions & 0 deletions packages/engine/src/HookExecution.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
const glob = typeof window === "undefined" ? global : window;

const overrideFunctions = [
"setTimeout",
"setInterval",
"console",
"EventTarget.prototype.addEventListener",
] as const;

const originalReferences: HookContext = {
setTimeout: glob.setTimeout,
setInterval: glob.setInterval,
console: glob.console,
"EventTarget.prototype.addEventListener":
glob.EventTarget.prototype.addEventListener,
};

export type HookContext = { [K in typeof overrideFunctions[number]]: any };

function setProperty(base: Object, path: string, value: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this? except for EventTarget all hooks are on global objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit overkill now, but I do like that we can keep a list of all the functions we override in the top of this file. Also added some comments, looks clean now imo

const layers = path.split(".");
if (layers.length > 1) {
const toOverride = layers.pop()!;
// Returns second last path member
const layer = layers.reduce((o, i) => o[i], base as any);
layer[toOverride] = value;
} else {
(base as any)[path] = value;
}
}

export class HookExecution {
public disposers: Array<() => void> = [];
public context: HookContext = {
setTimeout: this.createHookedFunction(setTimeout, this.disposers, (ret) => {
clearTimeout(ret);
}),
setInterval: this.createHookedFunction(
setInterval,
this.disposers,
(ret) => {
clearInterval(ret);
}
),
console: {
...originalReferences.console,
log: (...args: any) => {
// TODO: broadcast output to console view
originalReferences.console.log(...args);
},
},
["EventTarget.prototype.addEventListener"]: undefined,
};

constructor() {
if (typeof EventTarget !== "undefined") {
this.context["EventTarget.prototype.addEventListener"] =
this.createHookedFunction(
EventTarget.prototype.addEventListener as any,
this.disposers,
function (this: any, _ret, args) {
this.removeEventListener(args[0], args[1]);
}
);
}
}

public attachToWindow() {
overrideFunctions.forEach((path) =>
setProperty(glob, path, this.context[path])
);
}

public detachFromWindow() {
overrideFunctions.forEach((path) =>
setProperty(glob, path, originalReferences[path])
);
}

public dispose() {
this.disposers.forEach((d) => d());
}

private createHookedFunction<T, Y>(
original: (...args: T[]) => Y,
disposes: Array<() => void>,
disposer: (ret: Y, args: T[]) => void
) {
return function newFunction(this: any): Y {
const callerArguments = arguments;
const ret = original.apply(this, callerArguments as any); // TODO: fix any?
const ctx = this;
disposes.push(() => disposer.call(ctx, ret, callerArguments as any));
return ret;
};
}
}
26 changes: 11 additions & 15 deletions packages/engine/src/executor.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { autorun, runInAction } from "mobx";
import { TypeCellContext } from "./context";
import { installHooks } from "./hookDisposables";
import { HookExecution } from "./HookExecution";
import { Module } from "./modules";
import { isStored } from "./storage/stored";
import { isView } from "./view";
Expand Down Expand Up @@ -61,6 +61,7 @@ export async function runModule(
mod: Module,
context: TypeCellContext<any>,
resolveImport: (module: string) => any,
hookExecution: HookExecution,
beforeExecuting: () => void,
onExecuted: (exports: any) => void,
onError: (error: any) => void,
Expand Down Expand Up @@ -96,7 +97,7 @@ export async function runModule(
);
}

const execute = async () => {
async function execute(this: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't remember why I changed it. I can revert it back if you prefer the const

try {
if (wouldLoopOnAutorun) {
detectedLoop = true;
Expand All @@ -114,25 +115,20 @@ export async function runModule(
disposeEveryRun.length = 0; // clear existing array in this way, because we've passed the reference to resolveDependencyArray and want to keep it intact

beforeExecuting();
const hooks = installHooks();
disposeEveryRun.push(hooks.disposeAll);
let executionPromise: Promise<any>;

disposeEveryRun.push(hookExecution.dispose.bind(hookExecution));
hookExecution.attachToWindow();

try {
executionPromise = mod.factoryFunction.apply(
undefined,
argsToCallFunctionWith
); // TODO: what happens with disposers if a rerun of this function is slow / delayed?
await mod.factoryFunction.apply(undefined, argsToCallFunctionWith);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we can introduce an await here, because we can start having race conditions.

If the user function contains a long timeout, other code can execute in the meantime and we'd have it falsely "hooked".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A yes, goed gespot!

} finally {
// Hooks are only installed for sync code. Ideally, we'd want to run it for all code, but then we have the chance hooks will affect other parts of the TypeCell (non-user) code
// (we ran into this that notebooks wouldn't be saved (_.debounce), and also that setTimeout of Monaco blink cursor would be hooked)
hooks.unHookAll();
hookExecution.detachFromWindow();

if (previousVariableDisposer) {
previousVariableDisposer(exports);
}
}

await executionPromise;

// Running the assignments to `context` in action should be a performance improvement to prevent triggering observers one-by-one
wouldLoopOnAutorun = true;
runInAction(() => {
Expand Down Expand Up @@ -211,7 +207,7 @@ export async function runModule(
//reject(e);
resolve();
}
};
}

const autorunDisposer = autorun(() => execute());

Expand Down
65 changes: 0 additions & 65 deletions packages/engine/src/hookDisposables.ts

This file was deleted.

12 changes: 11 additions & 1 deletion packages/engine/src/modules.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { TypeCellContext } from "./context";
import { observable, untracked, computed, autorun } from "mobx";
import { HookContext } from "./HookExecution";
import { set as setProperty } from "lodash";
// import { stored } from "./storage/stored";
// import { view } from "./view";

Expand Down Expand Up @@ -71,7 +73,10 @@ function createDefine(modules: Module[]) {
};
}

export function createExecutionScope(context: TypeCellContext<any>) {
export function createExecutionScope(
context: TypeCellContext<any>,
hookContext: HookContext
) {
const scope = {
autorun,
$: context.context,
Expand All @@ -83,6 +88,11 @@ export function createExecutionScope(context: TypeCellContext<any>) {
// view,
observable,
};

Object.keys(hookContext).forEach((path) =>
setProperty(scope, path, hookContext[path as keyof HookContext])
);

return scope;
}

Expand Down