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

[Proposal] fix: use friendlier promise checking #84

Merged
merged 2 commits into from
Aug 19, 2022

Conversation

swain
Copy link
Contributor

@swain swain commented May 16, 2022

Motivation

The current syntax is not friendly to scenarios where the global Promise object may have been re-mapped to a different class, and async functions are being used. Example:

class WrappedPromise {
  // ...
}

global.Promise = WrappedPromise;

const asyncFunction = async () => 'something';

const result = asyncFunction();

console.log(result instanceof Promise);

This syntax should be more resilient against weird Promise scenarios, including this one.

@swain swain changed the title fix: use friendlier promise checking [Proposal] fix: use friendlier promise checking May 16, 2022
@swain swain marked this pull request as ready for review May 16, 2022 23:47
@swain
Copy link
Contributor Author

swain commented May 16, 2022

@fengmk2 would love any feedback on this PR

@swain
Copy link
Contributor Author

swain commented May 31, 2022

@fengmk2 bumping this PR :)

@fengmk2
Copy link
Member

fengmk2 commented Jun 2, 2022

@smolster Can you add a test case to cover this change?

@fengmk2 fengmk2 self-assigned this Jun 2, 2022
index.js Outdated
@@ -62,17 +62,15 @@ module.exports = function(options) {

let origin;
if (typeof options.origin === 'function') {
origin = options.origin(ctx);
if (origin instanceof Promise) origin = await origin;
origin = await Promise.resolve().then(() => options.origin(ctx));
Copy link
Member

Choose a reason for hiding this comment

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

origin could be a normal function, why we need to change it to a promise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're asking "why would we use Promise.resolve().then(...)" instead of just using await options.origin(ctx). I updated to use that simpler syntax.

If that's not what you're asking, could you elaborate a little more on your question?

@swain
Copy link
Contributor Author

swain commented Jun 4, 2022

@fengmk2 tests added 👍🏻 -- apologies for leaving it off at first.

@swain swain requested a review from fengmk2 June 4, 2022 19:42
@swain swain force-pushed the friendlier-promise-checking branch from 1a5527d to 22c3d11 Compare June 4, 2022 19:42
@swain
Copy link
Contributor Author

swain commented Jun 10, 2022

@fengmk2 bumping this fix

@swain
Copy link
Contributor Author

swain commented Jun 27, 2022

@fengmk2 any update on this PR?

@swain
Copy link
Contributor Author

swain commented Jul 11, 2022

@fengmk2 coming up on 2 months since this PR has been opened. Is it safe to merge now?

@swain
Copy link
Contributor Author

swain commented Aug 18, 2022

@fengmk2 3 months since this PR opened :/ -- think we can merge it?

@fengmk2 fengmk2 merged commit c4b5d21 into koajs:master Aug 19, 2022
@fengmk2
Copy link
Member

fengmk2 commented Aug 19, 2022

@swain I will add more tests before npm publish.

@fengmk2
Copy link
Member

fengmk2 commented Aug 19, 2022

3.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants