lib: add diagnostics_channel events to module loading

[Qard]

Adding module.cjs.load and module.esm.load diagnostics_channel events so APMs don't need to patch the module system to know when a particular module has been loaded.

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[bmeck]

I'd be more comfortable not sending something that will be directly patched as it won't work for ESM

…

On Mon, Aug 22, 2022, 7:39 AM Stephen Belanger ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/internal/modules/cjs/loader.js <#44340 (comment)>: > @@ -797,6 +800,7 @@ Module._load = function(request, parent, isMain) { throw new ERR_UNKNOWN_BUILTIN_MODULE(request); } + onModuleLoad.publish(module); The hasSubscribers check is only relevant if you're building input objects. If you just pass stuff in as-is it will be a no-op when there's no subscribers. Do we want to match anyway though just for consistency? — Reply to this email directly, view it on GitHub <#44340 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJI2NJF3NWVW462NJJ3TV2NYJNANCNFSM57HMHFCQ> . You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***>

[GeoffreyBooth]

Since the ESM loader can also load CommonJS, should the event perhaps just be "module load" without the specific of whether it's ESM or CommonJS? I assume there's a payload with details like what module was loaded (?) so that could be where we specify which loader was used, which format the file was recognized as, etc.

[GeoffreyBooth]

Can you write a test in ESM (in an .mjs file) to confirm that works too? What about static import statements?

[Qard]

Not sure how that would work. Wouldn't the module graph already be resolved/loaded by the time any code runs to actually register a subscriber? Maybe it could work in a loader?

[GeoffreyBooth]

That's why I asked 😄

We should add a test that shows expected esm behavior.

[simon-id]

Don't forget to add it to the docs here: https://nodejs.org/api/diagnostics_channel.html#built-in-channels

[rochdev]

Could more events be added? There are use cases where having just one event is not enough, for example capturing how long each require call took which would require 2 events surrounding the function. Also including more data than just the module would be helpful, for example the path to the entry point, whether it's a core module, the base path, etc. Basically most of what require-in-the-middle has been doing historically, except the version since that's specific to package managers. Basically I think there could be 3 events: 1 when require is called, 1 when it finishes, and 1 when the module is actually loaded with module resolution information included as well.

[trevnorris]

@Qard Thanks for doing this. I just want to let you know that this would cover all the use cases for the data we've been collecting internally.

[trevnorris]

@Qard Would you mind fixing the lint error so all the tests can pass?

[Qard]

There's some more changes I've been meaning to make. I'll try to get to that after NodeConf EU. Just had a very busy few weeks. 😅

[GeoffreyBooth]

There’s some more changes I’ve been meaning to make. I’ll try to get to that after NodeConf EU. Just had a very busy few weeks. 😅

Thanks. Let’s also please not land this until all open questions are resolved.

[Qard]

Going to redo this on #44943 after it lands.

[GeoffreyBooth]

Going to redo this on #44943 after it lands.

Thanks. Will that other PR enable things like I mentioned in #44340 (comment), where we could have more generic events like “module load” and then additional properties for extra data like module specifier, module system (CommonJS or ESM), resolved path, etc.?

[Qard]

I've reimplemented this on top of #44943. Ignore the first commit, that's inherited from that PR. I'll rebase after that lands to fix this PR. :)

[GeoffreyBooth]

FYI there's a lot of refactoring in progress for module loading, including starting to merge together the CJS and ESM loaders. I would wait until the dust settles on that work before landing this. I would also suggest you make the events less tied to subsystem: like module resolution, module loading; whether it was from require or import, etc.

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/44340
✔ Done loading data for nodejs/node/pull/44340
----------------------------------- PR info ------------------------------------
Title lib: add diagnostics_channel events to module loading (#44340)
Author Stephen Belanger (@Qard)
Branch Qard:diagnostics-channel-for-module-loads -> nodejs:main
Labels module, semver-minor, esm, author ready, needs-ci, loaders, diagnostics_channel, commit-queue-squash
Commits 1
- lib: add diagnostics_channel events to module loading
Committers 1
- RafaelGSS PR-URL: https://github.com/nodejs/node/pull/44340
Reviewed-By: Geoffrey Booth Reviewed-By: Santiago Gimeno Reviewed-By: Rafael Gonzaga Reviewed-By: James M Snell Reviewed-By: Gerhard Stöbich ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44340
Reviewed-By: Geoffrey Booth Reviewed-By: Santiago Gimeno Reviewed-By: Rafael Gonzaga Reviewed-By: James M Snell Reviewed-By: Gerhard Stöbich --------------------------------------------------------------------------------
⚠ Commits were pushed since the last approving review:
⚠ - lib: add diagnostics_channel events to module loading
ℹ This PR was created on Mon, 22 Aug 2022 11:57:11 GMT
✔ Approvals: 5
✔ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/44340#pullrequestreview-2126574661
✔ - Santiago Gimeno (@santigimeno): https://github.com/nodejs/node/pull/44340#pullrequestreview-2126639621
✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/44340#pullrequestreview-2126661939
✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/44340#pullrequestreview-2128947843
✔ - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/44340#pullrequestreview-2129682835
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2024-06-20T17:43:17Z: https://ci.nodejs.org/job/node-test-pull-request/59906/
- Querying data for job/node-test-pull-request/59906/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/9605921698

[RafaelGSS]

Landed in fbdfe93

[joyeecheung]

Tentatively backport in #56927 to reduce the amount of conflicts.

[joyeecheung]

Backing it out from #56927 to avoid introducing surface for regressions.