Skip to content

feat: add support for synchronous module.registerHooks#27

Merged
timfish merged 2 commits into
apm-js-collab:mainfrom
isaacs:main
May 18, 2026
Merged

feat: add support for synchronous module.registerHooks#27
timfish merged 2 commits into
apm-js-collab:mainfrom
isaacs:main

Conversation

@isaacs

@isaacs isaacs commented May 15, 2026

Copy link
Copy Markdown
Contributor

Also, add coverage for the debug dump path, and c8 ignore some lines that were being mysteriously uncovered.

I'm not sure why, but tests don't actually run in node 26, due to yargs being interpreted as ESM and loaded improperly from c8.

This will allow us to set up Orchestrion in node versions that deprecate module.register, without spamming stderr with warnings.

@jsumners-nr

Copy link
Copy Markdown
Contributor

I'm not sure why, but tests don't actually run in node 26, due to yargs being interpreted as ESM and loaded improperly from c8.

nodejs/node#62083 (comment)

@isaacs isaacs force-pushed the main branch 2 times, most recently from 7a2a136 to 536ca96 Compare May 15, 2026 16:48
@isaacs

isaacs commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

Playing with this, it looks like the sync module.registerHooks hooks in node 22 are present, but buggy. I'm going to add a check (in the example code in the docs) to prevent it from being used in that version, rather than only relying on feature detection.

@isaacs isaacs force-pushed the main branch 2 times, most recently from bfa072a to d248d00 Compare May 15, 2026 19:21

@bizob2828 bizob2828 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks for adding support for registerHooks @isaacs!

@isaacs

isaacs commented May 18, 2026

Copy link
Copy Markdown
Contributor Author

I did some research and just force-pushed an update to the docs. (The code is as it was reviewed, no further changes there.) Had to do a bunch of testing to make sure the assumptions were create, but the readme now just specifies a single path forward that works for every situation. I feel like anyone who needs to use strictly --require in old node versions, probably is knowledgeable enough to figure out how to make that work.

Module.register() really was such a messy experiment, hopefully it can rest in peace moving forward 😅

@timfish timfish changed the title feat: add support for synchronous module.registerHooks feat: add support for synchronous module.registerHooks May 18, 2026
@timfish timfish merged commit abad7ae into apm-js-collab:main May 18, 2026
3 checks passed
@timfish timfish mentioned this pull request May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants