Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hooking parent and sub module with a single hook #84

Merged

Conversation

pgayvallet
Copy link
Contributor

Fix #79

Fix the bug causing a hook used for both a module and some of its sub module to only hook into the parent module.

new Hook(['sub-module/foo', 'sub-module'], handler)

Copy link

@gsoldevila gsoldevila left a comment

Choose a reason for hiding this comment

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

LGTM!

index.js Outdated
return exports
}

if (modules.includes(fullModuleName)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (modules.includes(fullModuleName)) {
if (modules.includes(fullModuleName) && moduleName !== fullModuleName) {

This all looks good. I can't break it. :)

One thing that was niggling me a little bit is this one case. If moduleName === fullModuleName -- e.g. when doing require('sub-module') -- then this would result in (a) the isWhitelistedSubmodule === true being slightly misleading and (b) the if (!isWhitelistedSubmodule) { ... block below being skipped. However, I can't come up with a case where skipping that block isn't just fine -- I expect res === filename every time that happens.

tl;dr: What you have here works, as best I can tell, but I think I'd prefer to have this small patch.

Copy link
Member

Choose a reason for hiding this comment

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

Let me know if you have thoughts on the suggestion. Then I can get a release out of this soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think you're right: in practice, I'm not sure it's impacting in any way, but I agree that it's still technically better to dissociate that case.

I pushed your suggestion, let me know if you think we need to perform any more changes. Otherwise, I think we're good releasing as soon as you can.

@trentm trentm merged commit 9fb51f5 into elastic:main Mar 12, 2024
16 checks passed
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.

hooking 'sub-module/foo' does not work if 'sub-module' is also in the set of modules to hook
3 participants