Refactor Hooks

[niklaskors]

After a few failed attempts I came up with this solution. This solves the async problem for hooks and makes sure the hooked/overridden functions do not interfere with the rest of the codebase.

• The way of injecting is pretty ugly atm, maybe you can think of something better in: packages/engine/src/modules.ts

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/yousefed/typecell-next/6Po1WeDxLKcqjkJsbXfURQR9XbPG
✅ Preview: https://typecell-next-git-hook-refactor-yousefed.vercel.app

[YousefED]

Let's distinguish 3 types of places where a hooked function can be called (we use setInterval as example):

code scenarios

1. Regular

let ret = 4;
setInterval(...)

2. Regular after await

let ret = 4;
await sleep(1);
setInterval(...);

3. External modules

// my-npm-timer-module
export function myCustomSetInterval(t, h) {
return window.setInterval(t, h);

import * as lib from "my-npm-timer-module"
let ret = 4;
lib.myCustomSetInterval(...);

4. External modules after await

import * as lib from "my-npm-timer-module"
let ret = 4;
await sleep(1);
lib.myCustomSetInterval(...)

The old approach captures 1 and 3. The new approach captures 1 and 2.

Note that none of the approaches capture scenario 4. We could try to fix it by injecting a call to installHooks after every await. But that still won't work if my-npm-timer-module makes multiple await calls internally. I think for now it's ok to say that we won't be able to fix this scenario (maybe until something like async_hooks becomes available).

use cases

To decide on the approach, let's first conclude that we have two use-cases:

(1) Cleaning up of resources:

• When we reevaluate a cell, we need to make sure as much as possible that there are no old resources lying around. e.g.: if a setInterval of the old code is running, two code paths will keep executing indefinitely

(2) Adding diagnostics:

• We want to capture console messages so we can provide better diagnostics. For this scenario it's not necessary per se that code from my-npm-timer-module would be hooked as well, but I think it would be nice. It's more important that scenario 1 and 2 are covered.

---

I think when looking at this, we can make a solution that covers 1,2 and 3, right? That's already a good improvement!

[YousefED]

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

[niklaskors]

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");
});

[YousefED]

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

[niklaskors]

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

[YousefED]

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

[niklaskors]

Solved if you also inject window :)

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

[niklaskors]

Good point on the cleaning up of resources. That's definitely the biggest caveat of this approach because the hooks won't work inside npm modules and therefore won't clean up it's resources.

• So a quick fix would be to still synchronously set window.setTimeout & window.setInterval upon execution and remove them after to support this for modules. (note that this still breaks when a library invokes these methods somewhere async)
• Alternative: Try to inject hook references into library code
• Far fetched idea: execute code inside web workers so everything is executed within a new window

Or do you already have some magic in mind? 🌈

[YousefED]

• So a quick fix would be to still synchronously set window.setTimeout & window.setInterval upon execution and remove them after to support this for modules. (note that this still breaks when a library invokes these methods somewhere async)

Yes, so that's the old solution right?

Alternative: Try to inject hook references into library code

Going to be nasty so let's not go down this rabbit hole for as long as possible :)

Far fetched idea: execute code inside web workers so everything is executed within a new window

Yep that would be a nice isolated scope. But you also won't have access to things like window / document, so it's a no go for now

[YousefED]

Looks good! Could use come comments and tests ofc

[YousefED]

Some comments. I think we'll also need some good tests for this :)

Either via Jest, or via playwright-test. It's now possible to create unittests that run in Playwright (see imports.browsertest.ts)

[YousefED]

Is there a reason for this change?

[niklaskors]

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

[YousefED]

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

[niklaskors]

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

[YousefED]

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".

[niklaskors]

A yes, goed gespot!

[niklaskors]

Some comments. I think we'll also need some good tests for this :)

Either via Jest, or via playwright-test. It's now possible to create unittests that run in Playwright (see imports.browsertest.ts)

I added some tests for this on engine level which tests it for console: https://github.com/YousefED/typecell-next/pull/310/files#diff-835bb6182fb8cb597600b1c532824a0491cafcd317b8cad81ea16b152b1354c8
I think that covers the basics

[YousefED]

This looks good. My personal taste would be to remove setProperty in HookExecution.ts, and for example introduce a Hook class / object who's own responsibility it is to hook/unhook the right function. So for EventTarget it would be a simple call of window.EventTarget.prototype.addEventListener = newFunction;

This would be a bit more repetitive (your implementation with setProperty is more abstract / less duplicate code. However, I think it would be a bit more readable / explicit, which is a benefit in this case as this is quite a complicated piece of code.

However, this is not a must have.

The main improvement would be some unit tests that cover (a) the different hooks and (b) the 4 scenarios outlined in the PR. Not sure how much effort this would be?