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
feat: implement auto preview on error #87
Conversation
✅ Deploy Preview for jest-preview-library canceled.
|
68c1332
to
0462e0a
Compare
@thanhsonng Can you double-check the implementation? Also, I rebased some of your commits, so please notice if you need to push to this branch. Thanks |
This implementation's currently not working yet. |
src/configure.ts
Outdated
console.log(callback.constructor.name); | ||
callbackWithPreview = async function () { | ||
try { | ||
return await (callback as () => Promise<unknown>)(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opportunity to improve: We can write a helper function to detect an async function, that will make TypeScript happy and doesn't require type assertion here. I will add an example below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thanhsonng I'm waiting for your example. You can commit directly as well if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out it is not easy to achieve. We need to inspect the return value, i.e. we must call the callback
function first.
On the other hand, checking callback.constructor.name === 'AsyncFunction'
is not enough IMO. We actually want to check if the function return a Promise.
So I propose something like this. We always await
for callback's return value, regardless of its signature, and we make callbackWithPreview
always return a Promise.
if (!callback) {
callbackWithPreview = undefined;
} else {
callbackWithPreview = async (...args: Parameters<jest.ProvidesCallback>) => {
try {
// @ts-ignore
return await callback(...args);
} catch (error) {
debug();
throw error;
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checking
callback.constructor.name === ‘AsyncFunction’
is not enough IMO. We actually want to check if the function return a Promise.
Can you elaborate why we need to check if the function return a Promise, instead of checking the function is an async function?
So I propose something like this. We always
await
for callback’s return value, regardless of its signature, and we makecallbackWithPreview
always return a Promise.
I did try it before this and it works actually (not well tested yet). However, I have a few opening questions I haven’t had answers yet:
- Is there any issues regarding the rightness when we always converting sync => async?
- Is there any performance issues when we always use async (even when not needed)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate why we need to check if the function return a Promise, instead of checking the function is an async function?
An async function is just syntactic sugar for functions that return a promise. Jest allows user to write functions that return a promise without using async/await syntax, so we must account for that case as well. This link is for reference: https://jestjs.io/docs/tutorial-async.
I did try it before this and it works actually (not well tested yet). However, I have a few opening questions I haven’t had answers yet:
- Is there any issues regarding the rightness when we always converting sync => async?
- Is there any performance issues when we always use async (even when not needed)?
I believe those are non-issues but you shouldn't take my words for it. I will do some research about these questions. 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lemme do a benchmark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thanhsonng I did a benchmark and the running time is very similar. So I will go with always async
approach.
Since we are missing a
it('test name', async () => {
// Never automatic preview if test fail
}) cc: @thanhsonng |
Co-authored-by: Thanh Son Nguyen <thanhsonng.211@gmail.com>
Features
autoPreview
optionChores