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.

Sign up for GitHub

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

utils: Override async functions with async functions #2163

Closed
wants to merge 1 commit into from
Closed

utils: Override async functions with async functions #2163

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,9 +322,16 @@ export class InjectionsHandler extends BasicHandler {
if (!(original instanceof Function))
throw new Error(`Virtual function ${name}() is not available for ${object}`);

object[name] = function (...args) {
return injectedFunction.call(this, original, ...args);
};
if (original.constructor.name === 'AsyncFunction') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, funny thing... I had this changed months ago and I forgot to commit...

However, define something like const AsyncFunctionType = (async () => {}).constructor; and then check for original instanceof AsyncFunctionType.

Also I feel we should check if the two things match, so if both the original ones are async or not and depending on that behave differently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I saw that suggestion when googling but it felt like a hack so I intentionally didn't do it that way. It's uglier, more code, and not obviously correct to me.

Good point about checking two things match though...

Since it turns out the bug #2110 actually isn't important, and it's fixed by #2166, and you have another fix in mind, I suggest just merging your own fix or wait for #2166.

// eslint-disable-next-line require-await
object[name] = async function (...args) {
return injectedFunction.call(this, original, ...args);
};
Comment on lines +327 to +329
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, if you're declaring this async and injectedFunction that would be meant to await, so I think you can just drop the async and return the promise as it is no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I expect #2110 will be solved either by #2166 or by your own commit you said you had, or both. So we don't need to continue here.

} else {
object[name] = function (...args) {
return injectedFunction.call(this, original, ...args);
};
}
return [object, name, original];
}

Expand Down
Loading