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

Refactor Hooks #295

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.scopeHooks
);
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
102 changes: 102 additions & 0 deletions packages/engine/src/HookExecution.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
const glob = typeof window === "undefined" ? global : window;

// These functions will be added to the scope of the cell
const onScopeFunctions = ["setTimeout", "setInterval", "console"] as const;

// These functions will be attached to the window during cell execution
const onWindowFunctions = [
...onScopeFunctions,
"EventTarget.prototype.addEventListener",
] as const;

export const originalReferences: ScopeHooks & WindowHooks = {
setTimeout: glob.setTimeout,
setInterval: glob.setInterval,
console: glob.console,
"EventTarget.prototype.addEventListener":
glob.EventTarget.prototype.addEventListener,
};

export type ScopeHooks = { [K in typeof onScopeFunctions[number]]: any };

export type WindowHooks = { [K in typeof onWindowFunctions[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 scopeHooks: ScopeHooks = {
setTimeout: this.createHookedFunction(setTimeout, (ret) => {
clearTimeout(ret);
}),
setInterval: this.createHookedFunction(setInterval, (ret) => {
clearInterval(ret);
}),
console: {
...originalReferences.console,
log: (...args: any) => {
// TODO: broadcast output to console view
originalReferences.console.log(...args);
},
},
};

public windowHooks: WindowHooks = {
...this.scopeHooks,
["EventTarget.prototype.addEventListener"]: undefined,
niklaskors marked this conversation as resolved.
Show resolved Hide resolved
};

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

public attachToWindow() {
niklaskors marked this conversation as resolved.
Show resolved Hide resolved
onWindowFunctions.forEach((path) =>
setProperty(glob, path, this.windowHooks[path])
);
}

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

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

private createHookedFunction<T, Y>(
original: (...args: T[]) => Y,
disposer: (ret: Y, args: T[]) => void
) {
const self = this;
return function newFunction(this: any): Y {
const callerArguments = arguments;
const ret = original.apply(this, callerArguments as any); // TODO: fix any?
const ctx = this;
self.disposers.push(() =>
disposer.call(ctx, ret, callerArguments as any)
);
return ret;
};
}
}
21 changes: 12 additions & 9 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,18 +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);

disposeEveryRun.push(() => hookExecution.dispose());
hookExecution.attachToWindow();

let executionPromise: Promise<any>;

try {
executionPromise = mod.factoryFunction.apply(
undefined,
argsToCallFunctionWith
); // TODO: what happens with disposers if a rerun of this function is slow / delayed?
);
} 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);
}
Expand Down Expand Up @@ -211,7 +214,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.

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

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

export function createExecutionScope(context: TypeCellContext<any>) {
export function createExecutionScope(
context: TypeCellContext<any>,
hookContext: ScopeHooks
) {
const scope = {
autorun,
$: context.context,
Expand All @@ -82,7 +86,9 @@ export function createExecutionScope(context: TypeCellContext<any>) {
// stored,
// view,
observable,
...hookContext,
};

return scope;
}

Expand Down