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

const overrideFunctions = [
"setTimeout",
"setInterval",
"console",
"EventTarget",
"window",
"global",
] as const;

const originalReferences: HookContext = {
setTimeout: glob.setTimeout,
setInterval: glob.setInterval,
console: glob.console,
EventTarget: glob.EventTarget,
window: window,
global: global,
};

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

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

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

if (typeof window !== "undefined") {
this.context.window = {
...window,
setTimeout: this.context.setTimeout,
setInterval: this.context.setInterval,
console: this.context.console,
EventTarget: this.context.EventTarget,
};
}

if (typeof global !== "undefined") {
this.context.global = {
...global,
setTimeout: this.context.setTimeout,
setInterval: this.context.setInterval,
console: this.context.console,
EventTarget: this.context.EventTarget,
};
}
}

public attachToWindow() {
overrideFunctions
.filter((name) => name !== "window" && name !== "global")
.forEach((name) => {
(glob as any)[name] = this.context[name];
});
}

public detachFromWindow() {
overrideFunctions
.filter((name) => name !== "window" && name !== "global")
.forEach((name) => {
(glob as any)[name] = originalReferences[name];
});
}

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

private applyDisposer<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);
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.

7 changes: 6 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 { HookContext } 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: HookContext
) {
const scope = {
autorun,
$: context.context,
Expand All @@ -82,6 +86,7 @@ export function createExecutionScope(context: TypeCellContext<any>) {
// stored,
// view,
observable,
...hookContext,
};
return scope;
}
Expand Down