Skip to content

Conversation

@sirlancelot
Copy link
Contributor

@sirlancelot sirlancelot commented Nov 7, 2025

Copy link
Owner

@mcous mcous left a comment

Choose a reason for hiding this comment

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

This looks great! I think there's a little bug in the arity check logic to fix. Also, I think it's really important that the calledWith types are updated to reflect ignoreExtraArgs: true, e.g.

const spy = vi.fn<(a: string, b: number) => boolean>()

// all these should pass type-checking
when(spy, { ignoreExtraArgs: true }).calledWith()
when(spy, { ignoreExtraArgs: true }).calledWith('hello')
when(spy, { ignoreExtraArgs: true }).calledWith('hello', 123)

This requires a bit of relatively involved typescript, so if it blows up the diff or anything, just let me know and I can take care of it

src/behaviors.ts Outdated
Comment on lines 193 to 202
// Check arity
const expectedArguments = behavior.args
const { ignoreExtraArgs } = behavior
if (expectedArguments.length !== actualArguments.length && !ignoreExtraArgs)
return false

// Check arguments
return expectedArguments.every((expectedArgument, index) => {
return equals(actualArguments[index], expectedArgument)
})
Copy link
Owner

@mcous mcous Nov 9, 2025

Choose a reason for hiding this comment

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

I like this arity check before the arguments - it makes me realize there's a subtle bug in the current implementation around explicit vs implicit undefined values

A few suggestions, in priority order

  • Should the check be > in the ignoreExtraArgs case?
  • I don't think these comments add any extra value to the code, let's lose them in favor of variable names
Suggested change
// Check arity
const expectedArguments = behavior.args
const { ignoreExtraArgs } = behavior
if (expectedArguments.length !== actualArguments.length && !ignoreExtraArgs)
return false
// Check arguments
return expectedArguments.every((expectedArgument, index) => {
return equals(actualArguments[index], expectedArgument)
})
const { args: expectedArguments, ignoreExtraArgs } = behavior
const isMatchingArgumentCount = ignoreExtraArgs
? expectedArguments.length <= actualArguments.length
: expectedArguments.length === actualArguments.length
if (!isMatchingArgumentCount) {
return false
}
return expectedArguments.every((expectedArgument, index) => {
return equals(actualArguments[index], expectedArgument)
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expectedArguments length is defined by the stub the user created with .calledWith(). This rehearsal is expected to be "correct", and so actualArguments length should exactly match in order for the stubbing to be satisfied, unless ignoreExtraArgs is true.

Copy link
Owner

Choose a reason for hiding this comment

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

Given the following specification of ignoreExtraArgs...

Check all arguments in calledWith to find a match, but past the arguments specified in calledWith, ignore any extra arguments in the call itself when determining the match

...withignoreExtraArgs set, what do you expect the following stubbing to do?

const spy = when(vi.fn(), { ignoreExtraArgs: true })
  .calledWith(undefined)
  .thenReturn('called')

const result = spy() // should this be `called` or `undefined`?

My gut says when(vi.fn(), { ignoreExtraArgs: true }).calledWith(undefined) should not match spy(). However, with a !== check, this does match.

Passing undefined explicitly is extremely similar to, but not technically exactly the same as, omitting an argument, so I think if the user says "I expect an explicit undefined" it should be respected, even if ignoreExtraArgs is set

@sirlancelot
Copy link
Contributor Author

I created a new interface StubWrapperFlexible which fixes the type discrepancy when stubbing fewer arguments than the function signature would normally require.

@sirlancelot sirlancelot force-pushed the feature/ignore-extra-args branch from c130e19 to a5ff0b7 Compare November 11, 2025 00:29
@sirlancelot
Copy link
Contributor Author

Ha, it is a bit more difficult than I originally thought 😅

@sirlancelot
Copy link
Contributor Author

Okay, everything should be working now. Not sure why the build failed though, looking at the logs all the tasks worked, and it works for me locally 🤷

@mcous mcous merged commit 9e8e0da into mcous:main Nov 11, 2025
22 checks passed
@mcous
Copy link
Owner

mcous commented Nov 11, 2025

Thanks for the contribution! I'll get this released shortly, once I fix some extra typing stuff I just noticed unrelated to this PR

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.

2 participants