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 3 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
27 changes: 12 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 { executeWithHooks } from "./hooks";
import { Module } from "./modules";
import { isStored } from "./storage/stored";
import { isView } from "./view";
Expand Down Expand Up @@ -96,7 +96,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 +114,22 @@ 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>;

const { executeModel, disposeHooks } = executeWithHooks(
(hookContext) =>
mod.factoryFunction.apply(hookContext, argsToCallFunctionWith) // TODO: what happens with disposers if a rerun of this function is slow / delayed?
);

disposeEveryRun.push(disposeHooks);

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

71 changes: 71 additions & 0 deletions packages/engine/src/hooks.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// These will be injected in the compiled function and link to hookContext
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that your solution doesn't work if I'd call window.console.log() in user code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved if you also inject window :)

      hookContext.window = {
        ...window,
        console: {
          ...console,
          log: (...args) => {
            console.log("CAPTURED!", ...args);
          },
        },
      };

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

function 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;
};
}

export function executeWithHooks<T>(
modelFunction: (hookContext: any) => Promise<T>
) {
const disposers: Array<() => void> = [];

const executeModel = async function (this: any) {
const hookContext: { [K in typeof overrideFunctions[number]]: any } = {
setTimeout: applyDisposer(setTimeout, disposers, (ret) => {
clearTimeout(ret);
}),
setInterval: applyDisposer(setInterval, disposers, (ret) => {
clearInterval(ret);
}),
console: {
...console,
log: (...args: any) => {
// TODO: broadcast output to console view
console.log(...args);
},
},
EventTarget: undefined,
};

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

return modelFunction(hookContext);
};

return {
executeModel,
disposeHooks: () => {
disposers.forEach((d) => d());
},
};
}
12 changes: 12 additions & 0 deletions 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 { overrideFunctions } from "./hooks";
// import { stored } from "./storage/stored";
// import { view } from "./view";

Expand Down Expand Up @@ -112,5 +113,16 @@ export function getModulesFromTypeCellCode(compiledCode: string, scope: any) {
"$1async function"
); // TODO: remove await?

// Adds overridden functions
// TODO: improve injection method
totalCode = totalCode.replace(
/("use strict";)/,
`"use strict";
// Override functinos
${overrideFunctions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this logic the same as variableImportCode? i.e.: can't we just add the hooks to scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code inside scope will be inserted outside of the define module. In order to override global functions it needs to be injected inside the module.

So this doesn't work:

let console = this.console;
define(["require", "exports"], async function (require, exports) {
    "use strict";
    Object.defineProperty(exports, "__esModule", { value: true });
    console.log("Test");
});

But this does:

define(["require", "exports"], async function (require, exports) {
    "use strict";
    let console = this.console;
    Object.defineProperty(exports, "__esModule", { value: true });
    console.log("Test");
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure? I doubt this tbh (that option 1 doesn't work)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think I know why. It's because I didn't feed the hookContext into scope. Will fix :)

.map((hook: string) => `let ${hook} = this.${hook};`)
.join("\n")}\n`
);

return getModulesFromCode(totalCode, scope);
}