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: do nothing if target does not exist in getters map #155

Merged

Conversation

karrui
Copy link
Contributor

@karrui karrui commented Sep 27, 2024

#153 introduced the getters map to allow for multiple hooks. However, there may exist edge cases where the name does not exist in the target, resulting in the following error:

/project/path/node_modules/import-in-the-middle/lib/register.js:20
    return getters.get(target)[name]()
                                    ^

TypeError: getters.get(...)[name] is not a function
    at Object.get (/project/path/node_modules/import-in-the-middle/lib/register.js:20:37)
    at /project/path/node_modules/dd-trace/packages/datadog-instrumentations/src/helpers/hook.js:41:40
    at callHookFn (/project/path/node_modules/import-in-the-middle/index.js:29:22)
    at Hook._iitmHook (/project/path/node_modules/import-in-the-middle/index.js:150:11)
    at /project/path/node_modules/import-in-the-middle/lib/register.js:37:31
    at Array.forEach (<anonymous>)
    at register (/project/path/node_modules/import-in-the-middle/lib/register.js:37:15)
    at file:///project/path/node_modules/vitest/dist/chunks/base.BlXpj3e_.js?iitm=true:122:1
    at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:323:24)

Node.js v20.12.2

I am unable to trace why this happens, and thus am unable to create a test case. However, a hooked package resulted in the following target and name values when I logged it.

console.log(JSON.stringify(target), name) // {} default

typeof getters.get(target)[name] could also be undefined and cause a type error. I've made the type check stricter before invoking the function.

See DataDog/dd-trace-js#4713 (comment) for context

@timfish
Copy link
Contributor

timfish commented Sep 27, 2024

Sorry the changes are causing issues, the test cases and libraries we cover are quite extensive!

Do you know what library or module cause this? I see a couple of large projects linked that I can test.

@karrui
Copy link
Contributor Author

karrui commented Sep 27, 2024

Feel free to fork https://github.com/opengovsg/starter-kit and test. The current vitest tests are failing due to v1.11.1.

@timfish timfish self-assigned this Sep 27, 2024
lib/register.js Outdated Show resolved Hide resolved
Co-authored-by: Brad Adams <hi@breadadams.com>
lib/register.js Outdated Show resolved Hide resolved
@timfish
Copy link
Contributor

timfish commented Sep 27, 2024

I haven't debugged this yet but guess this is caused by the code checking for a default export and there is none so it should return undefined rather than throw.

@timfish
Copy link
Contributor

timfish commented Sep 27, 2024

I think a suitable test case could be added to the the end of test I added and it would check for an export that doesn't exist and confirm it's undefined without throwing.

const imp = await import('../fixtures/foo.mjs')

strictEqual(imp.foo(), 'foo-first-second')
// This should not throw!
strictEqual(imp.doesNotExist, undefined)

@karrui
Copy link
Contributor Author

karrui commented Sep 27, 2024

I think a suitable test case could be added to the the end of test I added and it would check for an export that doesn't exist and confirm it's undefined without throwing.

const imp = await import('../fixtures/foo.mjs')

strictEqual(imp.foo(), 'foo-first-second')
// This should not throw!
strictEqual(imp.doesNotExist, undefined)

That test case does not fail even before this fix, so might not be useful to add!

@karrui
Copy link
Contributor Author

karrui commented Sep 27, 2024

Found a test case that passes with this fix but not previously. Unsure if that's expected behaviour though. Will leave it to you to decide

@bizob2828
Copy link

bizob2828 commented Sep 27, 2024

I have confirmed this PR fixes the issue detailed in here. I know I don't have approver rights but I work with @jsumners who is on vacation

@timfish timfish requested a review from AbhiPrasad September 28, 2024 16:56
karrui added a commit to opengovsg/isomer that referenced this pull request Sep 30, 2024
`import-in-the-middle` v1.11.0 release broke datadog's vitest test visibility tracing. 

This PR temporarily disables the visibility harness for unit tests (playwright not affected) until it's fixed.

Related:
* DataDog/dd-trace-js#4732
* nodejs/import-in-the-middle#155


## Solution

This change disables the Datadog Test Visibility configuration and tracing in the CI workflow for the isomer-studio project.

**Breaking Changes**

- [x] No - this PR is backwards compatible

**Improvements**:

- Removed Datadog Test Visibility configuration step from the CI workflow
- Commented out Datadog-related environment variables for the test run

## Tests

No new tests are required for this change. Existing tests should continue to run without Datadog tracing.
@timfish
Copy link
Contributor

timfish commented Sep 30, 2024

Thanks @bizob2828. We had agreed on at least 2 approvals before merging so I'd still like an approval from another maintainer before merging!

Copy link

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for fixing this 👍

@timfish timfish merged commit 5f6be49 into nodejs:main Sep 30, 2024
48 checks passed
@jsumners-nr jsumners-nr mentioned this pull request Oct 3, 2024
@karrui karrui deleted the do-nothing-if-target-does-not-exist-in-getter-map branch December 12, 2024 09:15
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