Skip to content

fix: properly hook submodule package exports#215

Merged
AbhiPrasad merged 2 commits intonodejs:mainfrom
isaacs:isaacs/hook-submodule-exports
Dec 18, 2025
Merged

fix: properly hook submodule package exports#215
AbhiPrasad merged 2 commits intonodejs:mainfrom
isaacs:isaacs/hook-submodule-exports

Conversation

@isaacs
Copy link
Contributor

@isaacs isaacs commented Dec 17, 2025

This requires passing the specifier down the chain to the hook function,
in order to apply the hook function when it matches.

By the time the hook is being matched, the submodule specifier is
already overwritten to a full file:// URL, and appears as if the user
is requesting an arbitrary internal within the package by absolute path.
Since the module detected in that path does not match any of the module
specifiers, the hook would previously be skipped unless the internals: true option was set, and the submodule export path matched the file
subpath exactly.

With the original specifier present, the hook can be applied, by
matching the specifier requested by the user to the submodule specifier
provided at hook creation.

This is far less brittle as a hook author than using internals: true,
because it does not depend on the internal structure of the package, but
rather only the public API surface, which is much less likely to change.

Also: fix the test:e2e test. Turns out exports['module.exports'] is an alias for the default export on CJS modules when imported to ESM, as of node 23.

@isaacs isaacs force-pushed the isaacs/hook-submodule-exports branch from bd85297 to 84840b8 Compare December 17, 2025 16:50
@isaacs isaacs marked this pull request as ready for review December 17, 2025 16:51
@isaacs isaacs force-pushed the isaacs/hook-submodule-exports branch 2 times, most recently from 0100387 to 36866ee Compare December 17, 2025 19:07
This requires passing the specifier down the chain to the hook function,
in order to apply the hook function when it matches.

By the time the hook is being matched, the submodule specifier is
already overwritten to a full `file://` URL, and appears as if the user
is requesting an arbitrary internal within the package by absolute path.
Since the module detected in that path does not match any of the module
specifiers, the hook would previously be skipped unless the `internals:
true` option was set, _and_ the submodule export path matched the file
subpath exactly.

With the original specifier present, the hook can be applied, by
matching the specifier requested by the user to the submodule specifier
provided at hook creation.

This is far less brittle as a hook author than using `internals: true`,
because it does not depend on the internal structure of the package, but
rather only the public API surface, which is much less likely to change.
Bring test:e2e back to passing on node 23 and above, by recognizing that
a 'module.exports' export is now assigned as an alias for 'default'.
@isaacs isaacs force-pushed the isaacs/hook-submodule-exports branch from 36866ee to f431fef Compare December 17, 2025 19:17
@AbhiPrasad AbhiPrasad merged commit a20f47a into nodejs:main Dec 18, 2025
51 checks passed
@timfish
Copy link
Contributor

timfish commented Dec 19, 2025

This went out in v2.0.1!

@BridgeAR
Copy link
Member

We did not investigate what changed, but this does break dd-trace vitest instrumentation.

So while we might do something that is out of the norm (which I can not determine right now), it does break things and I wonder about the semverness.

@isaacs
Copy link
Contributor Author

isaacs commented Jan 8, 2026

@BridgeAR Do you have an example I could use to reproduce this? Even if you're doing something out of the norm, it seems like this shouldn't have been able to break anything that worked before, unless you were relying on not instrumenting a package submodule that was being explicitly requested to be instrumented.

isaacs added a commit to isaacs/import-in-the-middle that referenced this pull request Jan 8, 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