implement module.registerHooks() to run synchronous module customization hooks in thread #55698

joyeecheung · 2024-11-02T22:36:52Z

This introduces module.registerHooks() for registering module loader customization hooks that are run for all modules loaded by require(), import and functions returned by createRequire() in the same thread, which makes them easier for CJS monkey-patchers to migrate to.

import assert from 'node:assert';
import { registerHooks, createRequire } from 'node:module';
import { writeFileSync } from 'node:fs';

writeFileSync('./bar.js', 'export const id = 123;', 'utf8');

registerHooks({
  resolve(specifier, context, nextResolve) {
    const replaced = specifier.replace('foo', 'bar');
    return nextResolve(replaced, context);
  },
  load(url, context, nextLoad) {
    const result = nextLoad(url, context);
    return {
      ...result,
      source: result.source.toString().replace('123', '456'),
    };
  },
});

// Checks that it works with require.
const require = createRequire(import.meta.url);
const required = require('./foo.js');  // redirected by resolve hook to bar.js
assert.strictEqual(required.id, 456);  // replaced by load hook to 456

// Checks that it works with import.
const imported = await import('./foo.js');  // redirected by resolve hook to bar.js
assert.strictEqual(imported.id, 456);  // replaced by load hook to 456

This complements the module.register() hooks - the new hooks fit better internally and cover all corners in the module graph; whereas module.register() previously could not cover require() while it was in-thread, and still cannot cover createRequire() after being moved off-thread.

They are also run in the same thread as the modules being loaded and where the hooks are registered, which means they are easier to debug (no more console.log() getting lost) and do not have the many deadlock issues haunting the module.register() hooks. The new API also takes functions directly so that it's easier for intermediate loader packages to take user options from files that the hooks can't be aware of, like many existing CJS monkey-patchers do.

Background

This implements part of the proposal in nodejs/loaders#198 - for the motivations, see #52219 - this fills in the gap where being able to run in-thread and support require() is more important for a hook than being able to run asynchronous code (even then, packages like https://www.npmjs.com/package/everysync allows user to sync-ify async code off-thread themselves), especially for a large amount of CJS loader moneky-patchers out there to migrate to an officially supported API easily.

The synchronous, in-thread hooks are a lot easier to support universally in the loaders and reduce the number of dark corners where the hooks cannot run or behave in a surprising manner - I guess the amount of "this caveat of asynchronous hooks does not apply to the synchronous hooks" added in the documentation kind of also already proves the point...

For now only resolve and load are implemented to reach parity with module.register(). I left the exports hook to https://github.com/joyeecheung/node/tree/export-hooks which will be a follow-up when we decide the timing at which the post-load hook for CJS should run.

The commit module: use synchronous hooks for preparsing in import(cjs) isn't strictly necessary and could split into a follow-up instead, but I think it's nice to have it here instead of leaving out that corner uncovered.

nodejs-github-bot · 2024-11-02T22:36:57Z

Review requested:

@nodejs/loaders
@nodejs/startup

joyeecheung · 2024-11-19T15:07:57Z

Added a lot of tests. This should now be ready for review.

nodejs-github-bot · 2024-11-19T15:22:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63633/

codecov · 2024-11-19T16:36:32Z

Codecov Report

Attention: Patch coverage is 98.37278% with 11 lines in your changes missing coverage. Please review.

Project coverage is 88.54%. Comparing base (dbfcbe3) to head (9529784).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/cjs/loader.js	97.24%	6 Missing ⚠️
lib/internal/modules/esm/loader.js	94.44%	3 Missing ⚠️
lib/internal/modules/esm/translators.js	94.11%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55698      +/-   ##
==========================================
+ Coverage   88.50%   88.54%   +0.03%     
==========================================
  Files         656      657       +1     
  Lines      189261   189858     +597     
  Branches    36346    36451     +105     
==========================================
+ Hits       167508   168101     +593     
- Misses      14969    14976       +7     
+ Partials     6784     6781       -3

Files with missing lines	Coverage Δ
lib/internal/modules/customization_hooks.js	`100.00% <100.00%> (ø)`
lib/internal/modules/esm/module_job.js	`99.25% <100.00%> (+<0.01%)`	⬆️
lib/internal/modules/helpers.js	`98.84% <100.00%> (-0.01%)`	⬇️
lib/internal/modules/esm/translators.js	`92.93% <94.11%> (-0.17%)`	⬇️
lib/internal/modules/esm/loader.js	`97.80% <94.44%> (-0.23%)`	⬇️
lib/internal/modules/cjs/loader.js	`98.10% <97.24%> (-0.19%)`	⬇️

... and 32 files with indirect coverage changes

guybedford

We've discussed this in the past and my previous feedback still stands that overall I'm very much for this direction, with the caveat that we just need to ensure we guide hooks authors to patterns that work well for ensuring consistent behaviours including for environments that use workers.

Specifically it would be great if we can recommend the --import top-level hook registration pattern quite strongly, as that naturally works with workers and it's worth calling out that this does that explicitly when documenting registerHooks.

I do have one feature request I'd love to see here though - a one-way function to disable hooks for security module.lockHooks(), with no arguments, that disallows further register or registerHooks calls.

Something to this effect would be a huge help to ensure runtime hooks registered on arbitrary function calls is not a thing, and we do think about the execution model as "register hooks" moving into a stage of "hooks are registered and can't be registered", rather than thinking of hooks as being able to happen at any time.

doc/api/module.md

joyeecheung · 2024-11-19T19:05:10Z

I do have one feature request I'd love to see here though - a one-way function to disable hooks for security module.lockHooks(), with no arguments, that disallows further register or registerHooks calls.

That seems out of scope of this PR? If it was fine not having it for module.register, why does it need to happen in this PR?

guybedford · 2024-11-19T19:10:39Z

Having a hook lockdown phase is not a requirement for this PR by any means, but would be a nice to have to clarify the early hook registration model sooner rather than later.

joyeecheung · 2024-11-19T19:23:59Z

Having a hook lockdown phase is not a requirement for this PR by any means, but would be a nice to have to clarify the early hook registration model sooner rather than later.

I feel that should be something that's up to the discretion of the hook author, there can surely be use cases where one wants to avoid inheritance in third-party child processes/workers, or just to avoid infinite recursion in case the code run in the loader forks itself, though we could mention it in the documentation for sure.

guybedford · 2024-11-19T19:51:37Z

Right, so it's our job to educate hook authors how to write hooks to support multi threading (or not) and how to avoid recursive application etc. etc.

With regards to hook "lockdown" I think this is something that the application owner would care about - how do I know that an arbitrary npm package import isn't going to be doing hooks without me knowing about it? How do I know I'm not depending on a package that's automatically adding a "jsx" hook and changing the way my local code runs? A lockdown feature is for the application code owner's benefit. Ideally lockdown by default would be great too to ensure we don't enter a Node.js future where hooking the loader is common for library dependencies.

joyeecheung · 2024-11-19T19:57:14Z

Right, so it's our job to educate hook authors how to write hooks to support multi threading (or not) and how to avoid recursive application etc. etc.

While I agree better documentation should exist, I think we shouldn't really dive into too much details in the API documentation. It should either be in the learn section in the website, or just have dedicated repository for tutorials + examples like https://github.com/nodejs/node-addon-examples and the new https://github.com/nodejs/package-examples that we are spinning (I also left comment in this PR that I think the example loaders should be moved in its own repo, and refrained from adding too many complementary examples for the new API, I also noticed that some existing examples in the documentation didn't even work, but it felt out of scope to fix them here and IMO they should really just be tested properly in a dedicated repo).

guybedford · 2024-11-19T21:08:50Z

@joyeecheung to give an example, I was not aware --import applied for all threads and this came as a surprise to me personally. This information is valuable.

joyeecheung · 2024-11-20T01:28:57Z

Yes, though not all valuable information belongs in the API documentation ;) One can argue that https://nodejs.github.io/node-addon-examples/ is full of valuable information, but it's better to have a dedicated tutorial on a complex topic like this, instead of trying to elaborate on it in the API documentation. I think we can make do with one or two sentence about --import for this PR, and leave detailed explanation as a TODO, and don't pile even more information that should be better hosted elsewhere into the API documentation. If you take a look at https://nodejs.org/api/module.html you probably will also notice how tiring and unapproachable it is to have all that information piled into one page, people might never notice all the scattered details and recommendations simply because it's not a great read and kinda odd to have a tutorial taking 1/2 of the page, with the rest just being straightforward API documentation or just full of pointers to other docs instead of examples.

GeoffreyBooth

Great work! I’m excited to see this land.

GeoffreyBooth · 2024-11-20T05:02:53Z

doc/api/module.md

@@ -693,8 +703,24 @@ register('./my-hooks.mjs', {
 });
 ```

+### In-thread Synchronous Customization Hooks


I feel like we should list these first, since we intend these to be the way forward.

I think the docs should also be a bit direct and state that we intend to remove register once there is a migration path to registerHooks. With register still being categorized as “release candidate”, I wouldn’t want someone seeing register as more mature and more stable than registerHooks and therefore a safer choice.

I wouldn’t want someone seeing register as more mature and more stable than registerHooks and therefore a safer choice.

I think that is an accurate description for now, considering module.registerHooks() is not yet even released, and module.register() has already been used by several projects. The future migration path should be a follow up after we actually get some feedback from the user land. This is a semver-minor PR and we should keep it so. The problem of the off-thread hooks came from dictating a path and gearing too many architectural decisions towards it and closing all other doors before sufficient real-world feedback was collected, and I don't think we should make the same mistake again just because we believe "this time, we got it right!" (I hope we are, but this isn't up to us).

If you don’t want to include anything about “we consider this to be the way forward” just yet, fine, but I still think that the new hooks should go first; and the explanation about how the hooks compare with each other should live with the async hooks rather than with the sync hooks. That way when we remove the async hooks, the comparison text is already there and goes away cleanly.

I spent some time to move it and it took way to much effort to do an overhaul of the texts to describe the sync hooks first, because the existing texts always assume they are talking about the async ones and need wording changes everywhere. I don't think this needs to be done in the initial PR that's mostly just implementing things. Can you open a follow-up PR to do the rewrite?

GeoffreyBooth · 2024-11-20T05:04:31Z

doc/api/module.md

+`module.registerHooks()` is a lighter weight alternative to `module.register()`
+which takes hook functions directly and run them in the same thread where the
+modules are loaded. The hook functions are synchronous, which is necessary to


This won’t make much sense once module.register is removed. Perhaps we should just introduce registerHooks straightforwardly and leave any comparisons to the register section?

I think for people who are already using module.register() and trying to figure out what the new API offers, this should be noted. We can remove it when module.register() actually gets removed, which would take a while because the usage out there already warrants a deprecation cycle, and in that case this would also serve as what all deprecation needs in the documentation - how to migrate to the alternative of the deprecated API.

lib/internal/modules/cjs/loader.js

lib/internal/modules/sync_hooks.js

test/fixtures/module-hooks/log-user.mts

test/parallel/test-bootstrap-modules.js

test/parallel/test-module-hooks-import-wasm.mjs

test/parallel/test-module-hooks-preload.js

test/parallel/test-repl.js

jasnell · 2024-11-23T15:22:12Z

lib/internal/modules/sync_hooks.js

+  // TODO(joyeecheung): we may want methods that allow disabling/enabling temporarily
+  // which just sets the item in the array to undefined temporarily.
+  // TODO(joyeecheung): this can be the [Symbol.dispose] implementation to pair with
+  // `using` when the explicit resource management proposal is shipped by V8.


May as well add the Symbol.dispose implementation now to prepare since we have it available.

Is it needed for the first iteration? If not, I'd rather leave it out and just leave a TODO comment instead, lest this got derailed into another debate about whether that's the correct implementation.

Agreed this could be a follow-up. TBH, I don't really find how this could be helpful in the reality.

Yeah someone suggested this in the proposal PR IIRC, though I think using this with using would be a rare choice - if you want something temporarily customized in the same file, chances are you are going to anchor it to other smaller parts of a bigger hook that just stays in the chain?

legendecas · 2024-11-29T11:11:13Z

lib/internal/modules/sync_hooks.js

+  // TODO(joyeecheung): we may want methods that allow disabling/enabling temporarily
+  // which just sets the item in the array to undefined temporarily.
+  // TODO(joyeecheung): this can be the [Symbol.dispose] implementation to pair with
+  // `using` when the explicit resource management proposal is shipped by V8.


Agreed this could be a follow-up. TBH, I don't really find how this could be helpful in the reality.

lib/internal/modules/cjs/loader.js

guybedford

The readme context is looking a lot better, my main remaining suggestion would be to state the sync hooks first as the recommended path.

Furthermore, as far as I'm aware there is rough consensus to deprecate the async customization hooks. Are there plans to follow-up with a deprecation PR at all?

guybedford · 2024-11-30T20:33:15Z

doc/api/module.md

+1. `module.register(specifier[, parentURL][, options])` which takes a module that
+   exports asynchronous hook functions. The functions are run on a separate loader
+   thread.
+2. `module.registerHooks(options)` which takes synchronous hook functions that are
+   run directly on the thread where the module is loaded.


Why don't we just put the sync hooks first?

Because all the existing texts assume they are talking about the async hooks, so to avoid doing an wording overhaul of all the existing texts, the additional explanation of the sync hooks need to come second, which then also means the bullet points should come second otherwise the ordering looks rather off. I don't think this the wording overhaul has to be done in the initial PR that's mostly just implementing things. Can you open a follow-up PR to do the rewrite?

guybedford · 2024-11-30T20:33:31Z

doc/api/module.md

+1. Registering a file which exports a set of asynchronous hook functions, using the
+   [`register`][] method from `node:module`,
+2. Registering a set of synchronous hook functions using the [`registerHooks`][] method
+   from `node:module`.


Again, might be worth putting sync hooks first as the recommended path.

guybedford · 2024-11-30T20:33:57Z

doc/api/module.md

+The hooks can be registered before the application code is run by using the
+[`--import`][] or [`--require`][] flag:

 ```bash
 node --import ./register-hooks.js ./my-app.js
+node --require ./register-hooks.js ./my-app.js
 ```

 ```mjs
 // register-hooks.js
+// This file can only be require()-ed if it doesn't contain top-level await.
+// To register asynchronous, off-thread hooks, use module.register().
 import { register } from 'node:module';
-
 register('./hooks.mjs', import.meta.url);
 ```


Again, for this first example, it would be great to see the sync hooks workflow first, then have the alternative workflow for async hooks provided.

doc/api/module.md

nodejs-github-bot · 2024-12-09T10:19:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63947/

nodejs-github-bot · 2024-12-09T17:02:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63960/

nodejs-github-bot · 2024-12-09T19:14:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63963/

guybedford

Happy to leave the sync reordering to a deprecation PR for the async variant in due course.

nodejs-github-bot · 2024-12-09T21:03:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63964/

nodejs-github-bot · 2024-12-09T23:27:10Z

Landed in 2960a59...1215a8b

PR-URL: #55698 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

github-actions · 2024-12-10T15:00:17Z

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @joyeecheung.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

joyeecheung · 2024-12-10T15:16:59Z

Note to releasers: please use the OP text (modulo background) for summary in the change log. The graph can be left out I suppose.

This depends on at least two refactoring PRs that landed before: #54769 and #55590 .

PR-URL: #55698 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

Notable changes: crypto: * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142 doc: * stabilize util.styleText (Rafael Gonzaga) #56265 module: * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185 * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698 * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698 report: * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068 sqlite: * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213 src,lib: * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201 stream: * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096 PR-URL: #56310

Notable changes: crypto: * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142 doc: * stabilize util.styleText (Rafael Gonzaga) #56265 module: * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185 * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698 * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698 report: * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068 sqlite: * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213 src,lib: * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201 stream: * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096 PR-URL: TODO

Notable changes: crypto: * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142 doc: * stabilize util.styleText (Rafael Gonzaga) #56265 module: * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185 * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698 * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698 report: * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068 sqlite: * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213 src,lib: * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201 stream: * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096 PR-URL: #56310

Notable changes: crypto: * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142 dgram: * (SEMVER-MINOR) support blocklist in udp (theanarkh) #56087 doc: * stabilize util.styleText (Rafael Gonzaga) #56265 module: * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185 * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698 * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698 report: * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068 sqlite: * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213 src,lib: * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201 PR-URL: #56310

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Nov 2, 2024

joyeecheung force-pushed the sync-hooks-12 branch from d96846a to decfb68 Compare November 2, 2024 22:43

RedYetiDev added semver-minor PRs that contain new features and should be released in the next minor version. wip Issues and PRs that are still a work in progress. labels Nov 3, 2024

joyeecheung force-pushed the sync-hooks-12 branch from decfb68 to 7778156 Compare November 19, 2024 14:57

joyeecheung changed the title ~~[WIP] implement module.registerHooks() to run synchronous module customization hooks in thread~~ implement module.registerHooks() to run synchronous module customization hooks in thread Nov 19, 2024

joyeecheung marked this pull request as ready for review November 19, 2024 15:07

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 19, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 19, 2024

This comment was marked as outdated.

Sign in to view

joyeecheung force-pushed the sync-hooks-12 branch from 7778156 to 9d7b53a Compare November 19, 2024 15:22

guybedford reviewed Nov 19, 2024

View reviewed changes

doc/api/module.md Show resolved Hide resolved

doc/api/module.md Show resolved Hide resolved

GeoffreyBooth approved these changes Nov 20, 2024

View reviewed changes

jasnell reviewed Nov 23, 2024

View reviewed changes

joyeecheung removed the wip Issues and PRs that are still a work in progress. label Nov 27, 2024

legendecas reviewed Nov 29, 2024

View reviewed changes

guybedford reviewed Nov 30, 2024

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2024

This comment was marked as outdated.

Sign in to view

joyeecheung added 2 commits December 9, 2024 11:18

module: implement module.registerHooks()

b4af854

module: use synchronous hooks for preparsing in import(cjs)

9529784

joyeecheung force-pushed the sync-hooks-12 branch from 0f7172a to 9529784 Compare December 9, 2024 10:18

guybedford approved these changes Dec 9, 2024

View reviewed changes

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Dec 9, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 9, 2024

nodejs-github-bot closed this Dec 9, 2024

joyeecheung mentioned this pull request Dec 10, 2024

Proposal for a simple, universal module loader hooks API to replace require() monkey-patching #52219

Closed

joyeecheung added the notable-change PRs with changes that should be highlighted in changelogs. label Dec 10, 2024

aduh95 pushed a commit that referenced this pull request Dec 10, 2024

module: implement module.registerHooks()

e5bb6c2

PR-URL: #55698 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

joyeecheung mentioned this pull request Dec 12, 2024

module.registerHooks() tracking issue #56241

Open

11 tasks

github-actions bot mentioned this pull request Dec 18, 2024

2024-12-19, Version 23.5.0 (Current) #56310

Merged

implement module.registerHooks() to run synchronous module customization hooks in thread #55698

implement module.registerHooks() to run synchronous module customization hooks in thread #55698

Conversation

joyeecheung commented Nov 2, 2024 • edited Loading

Background

nodejs-github-bot commented Nov 2, 2024

joyeecheung commented Nov 19, 2024

This comment was marked as outdated.

nodejs-github-bot commented Nov 19, 2024

codecov bot commented Nov 19, 2024 • edited Loading

Codecov Report

guybedford left a comment

Choose a reason for hiding this comment

joyeecheung commented Nov 19, 2024

guybedford commented Nov 19, 2024

joyeecheung commented Nov 19, 2024

guybedford commented Nov 19, 2024

joyeecheung commented Nov 19, 2024 • edited Loading

guybedford commented Nov 19, 2024

joyeecheung commented Nov 20, 2024 • edited Loading

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

nodejs-github-bot commented Dec 9, 2024

nodejs-github-bot commented Dec 9, 2024

nodejs-github-bot commented Dec 9, 2024

guybedford left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 9, 2024

nodejs-github-bot commented Dec 9, 2024

github-actions bot commented Dec 10, 2024

joyeecheung commented Dec 10, 2024

joyeecheung commented Nov 2, 2024 •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading

joyeecheung commented Nov 19, 2024 •

edited

Loading

joyeecheung commented Nov 20, 2024 •

edited

Loading

joyeecheung Nov 21, 2024 •

edited

Loading

joyeecheung Nov 26, 2024 •

edited

Loading