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 not throw unhandled exception when data is undefined in interceptor.reply #4036

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

frederikprijck
Copy link

@frederikprijck frederikprijck commented Jan 31, 2025

This relates to...

N/A

Rationale

See below (Bug Fixes)

Changes

See below (Bug Fixes)

Features

N/A

Bug Fixes

When interceptor.reply would return { data: undefined }, it blows up in getResponseData:

TypeError [Error]: Cannot read properties of undefined (reading 'toString')
      at getResponseData (/Users/frederikprijck/Development/frederikprijck/undici/lib/mock/mock-utils.js:128:17)
      at TestContext.<anonymous> (/Users/frederikprijck/Development/frederikprijck/undici/test/mock-utils.js:180:26)
      at Test.runInAsyncScope (node:async_hooks:206:9)
      at Test.run (node:internal/test_runner/test:639:25)
      at Suite.processPendingSubtests (node:internal/test_runner/test:382:18)
      at Test.postRun (node:internal/test_runner/test:730:19)
      at Test.run (node:internal/test_runner/test:688:12)
      at async Suite.processPendingSubtests (node:internal/test_runner/test:382:7)

This got introduced here, more specifcally, this change

image

More specifically, in the before and after, data is not the same when it was undefined originally:

const { statusCode, data = '', responseOptions = {} } = {data: undefined}
console.log(data); // ''

vs

const replyParameters = { data: '', responseOptions: {}, ...{data: undefined} }
console.log(replyParameters.data) // undefined

As previously, when data was undefined, '' would be passed to getResponseData, and getResponseData would return '', I ensured we now also return '' when data being passed to getResponseData is undefined.

Breaking Changes and Deprecations

N/A

Status

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

mcollina commented Feb 2, 2025

CI seems failing

@frederikprijck
Copy link
Author

I am not sure the CI failure relates to my changes? Or at least, it isn't clear for me that it would. Any pointers?

@metcoder95
Copy link
Member

Failures seems unrelated, are the same ones failing in other PRs (e.g. #4044)

@frederikprijck
Copy link
Author

Hey @mcollina and @metcoder95, is there anything I can do to help progress this PR?

@mcollina
Copy link
Member

mcollina commented Feb 8, 2025

It seems windows CI is utterly broken. I'm not keen on landing any changes until that's sorted, and I haven't had time yet.

@frederikprijck
Copy link
Author

frederikprijck commented Feb 8, 2025

No worries at all. I entirely understand and I am not rushed in getting this out (we have worked around the behavior in our code-base, and don't need this change to unblock ourselves. I mainly just wanted to solve this for others that may encounter it). Just trying to follow up.

Take your time, I might drive by to follow up a few times, with no bad intentions to put any kind of pressure to land this, I entirly understand and appreciate you even willing to put the time to look into this eventually.

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.

3 participants