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

docs: add section about resolves and removal of Promise unwrapping #44

Merged
merged 6 commits into from
May 17, 2024

Conversation

sheremet-va
Copy link
Member

@sheremet-va sheremet-va commented May 17, 2024

No description provided.

Copy link

github-actions bot commented May 17, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/index.js 1.11 KB (0%) 23 ms (0%) 4 ms (-6.75% 🔽) 26 ms

@sheremet-va sheremet-va changed the title feat: do not await the returned promise feat!: do not await the returned promise May 17, 2024
@dubzzz
Copy link
Contributor

dubzzz commented May 17, 2024

I had this option: https://github.com/tinylibs/tinyspy/compare/main...dubzzz:return-src-p?expand=1.
It passes all the tests without any behaviour change except the promise not being rewrapped. I can open the PR if you prefer it!

@dubzzz
Copy link
Contributor

dubzzz commented May 17, 2024

@sheremet-va My option passes all the tests without any need to change them.

I should probably add the checks: expect(spy.results[0][1]).toBe(promise). So that it makes sure that it does not break anymore. Waiting your go to open or forget my potential PR. -> DONE on my Branch 🟢

@sheremet-va
Copy link
Member Author

sheremet-va commented May 17, 2024

Vitest team decided to make that change at our last weekly, we are introducing toHaveResolved matcher to ease the migration with this.

Your option has hidden bugs because it introduced uncaught exceptions that are hard to catch, this is how it worked before actually

@dubzzz
Copy link
Contributor

dubzzz commented May 17, 2024

Your option has hidden bugs because it introduced uncaught exceptions that are hard to catch, this is how it worked before actually (see #14).

Not really... The error is caught in my case. The .then receives two functions including one never throwing as a second argument. I think there is no risk of unhandled or I miss something 🤔

@dubzzz
Copy link
Contributor

dubzzz commented May 17, 2024

The #14 is not the same: by throwing in the catch it causes an unhandled. In my case I never throw for the catch clause and thus there is no unhandled.

@sheremet-va
Copy link
Member Author

sheremet-va commented May 17, 2024

The #14 is not the same: by throwing in the catch it causes an unhandled. In my case I never throw for the catch clause and thus there is no unhandled.

I see. Still, it incorrectly stores the awaited result instead of the value that was actually returned by the function. I would expect a Promise to be in calls.results here:

let promise
const mock = fn(() => {
  promise = new Promise((resolve) => resolve(42))
  return promise
})
mock.results[0] === promise
await mock
mock.results[0] === promise

@sheremet-va
Copy link
Member Author

sheremet-va commented May 17, 2024

We can combine your solution with this though and introduce .resolves (or .resolved) and .rejects (or .rejected) properties that have these values

@dubzzz
Copy link
Contributor

dubzzz commented May 17, 2024

Do you mean something like: dubzzz@deeec2d?

@sheremet-va
Copy link
Member Author

Do you mean something like: dubzzz@deeec2d?

Yes

@sheremet-va
Copy link
Member Author

Do you mean something like: dubzzz@deeec2d?

Yes

Oh, no, sorry. I mean:

result.then(
    (r: any) => (state.resolves.push(r)),
    (e: any) => (state.rejects.push(e))
)

@dubzzz
Copy link
Contributor

dubzzz commented May 17, 2024

If we go for:

result.then(
    (r: any) => (state.resolves.push(r)),
    (e: any) => (state.rejects.push(e))
)

There is a question we must answer:

Let's consider the following case:

const f = vi.fn(async (value) => {
  return value === 0 ? promiseWaitingFor5secReturning0 : promiseWaitingFor0secReturning1;
})
f(0);
f(1);

I imagine we want: resolves: [0, 1] and not [1, 0] (its the order in which they resolve).

@sheremet-va
Copy link
Member Author

sheremet-va commented May 17, 2024

Yeah, I would expect it to be resolved in the order they are called. This is how Promise.all returns its values

@sheremet-va
Copy link
Member Author

Another name would be settled or settledResults with PromiseFulfilledResult interface

@dubzzz
Copy link
Contributor

dubzzz commented May 17, 2024

I have something like: dubzzz@00a1862 (drafty version)

But I don't get why my state.resolves[resultIndex] = ['ok', r] does not make it 🤔

@dubzzz
Copy link
Contributor

dubzzz commented May 17, 2024

I have a fully working version with tests at https://github.com/tinylibs/tinyspy/compare/main...dubzzz:return-src-p?expand=1. Let me know if relevant for a PR or not.

@sheremet-va
Copy link
Member Author

I have a fully working version with tests at main...dubzzz:return-src-p?expand=1 (compare). Let me know if relevant for a PR or not.

PR welcome :)

@sheremet-va sheremet-va force-pushed the feat/dont-await-promise branch from 00b00ca to 0f87f85 Compare May 17, 2024 16:57
@sheremet-va sheremet-va changed the title feat!: do not await the returned promise docs: add section about resolves and removal of Promise unwrapping May 17, 2024
@sheremet-va sheremet-va merged commit 1b37529 into main May 17, 2024
2 checks passed
@sheremet-va sheremet-va deleted the feat/dont-await-promise branch May 17, 2024 17:00
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