Skip to content

fix: do not throw unhandled exception when data is undefined in interceptor.reply#4036

Merged
mcollina merged 3 commits intonodejs:mainfrom
frederikprijck:main
Feb 16, 2025
Merged

fix: do not throw unhandled exception when data is undefined in interceptor.reply#4036
mcollina merged 3 commits intonodejs:mainfrom
frederikprijck:main

Conversation

@frederikprijck
Copy link
Contributor

@frederikprijck frederikprijck commented Jan 31, 2025 •
edited
Loading

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

mcollina
mcollina approved these changes Feb 2, 2025
View reviewed changes
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
Contributor Author

frederikprijck commented Feb 3, 2025

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

metcoder95 commented Feb 4, 2025

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

@frederikprijck
Copy link
Contributor Author

frederikprijck commented Feb 7, 2025

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
Contributor Author

frederikprijck commented Feb 8, 2025 •
edited
Loading

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.

mcollina
mcollina approved these changes Feb 16, 2025
View reviewed changes
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 mcollina merged commit c14781c into nodejs:main Feb 16, 2025
30 of 31 checks passed
@github-actions github-actions bot mentioned this pull request Feb 28, 2025
Merged
@github-actions github-actions bot mentioned this pull request Mar 12, 2025
Closed
ronag added a commit to nxtedition/undici that referenced this pull request Mar 14, 2025
@ronag
Merge remote-tracking branch 'nodejs/main'
cd214d4 
* nodejs/main: (23 commits)
  Bumped v7.5.0 (nodejs#4091)
  Removed clients with unrecoverable errors from the Pool (nodejs#4088)
  feat: Allow disabling autoSelectFamily in an Agent (nodejs#4070)
  chore: update cache tests (nodejs#4027)
  fix: Fix retry-handler.js when retry-after header is a Date (nodejs#4084)
  feat: add mock call history to access request configuration in test (nodejs#4029)
  feat(docs): button to switch dark and light mode (nodejs#4044)
  Bumped v7.4.0 (nodejs#4071)
  fix: fix EnvHttpProxyAgent for the Node.js bundle (nodejs#4064)
  chore: update WPT (nodejs#4062)
  chore: update WPT (nodejs#4028)
  fix: handle missing vary header values (nodejs#4031)
  fix: do not throw unhandled exception when data is undefined in interceptor.reply (nodejs#4036)
  test: fix windows wpt (nodejs#4050)
  feat: mark `EnvHttpProxyAgent` as stable (nodejs#4049)
  don't check AbortSignal maxListeners on some node versions (nodejs#4045)
  feat(docs): copy to clipboard button (nodejs#4037)
  docs: fix incorrect method signature of `onResponseError` (nodejs#4030)
  docs: document about global dispatcher and errors (nodejs#3987) (nodejs#4014)
  chore: update WPT (nodejs#4011)
  ...
@github-actions github-actions bot mentioned this pull request May 12, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@metcoder95 metcoder95 metcoder95 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@frederikprijck @mcollina @metcoder95