Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Add async closure support #159

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

Merged
j-f1 merged 15 commits into main from jed/async-closure
Jun 5, 2022
Merged

Add async closure support #159

j-f1 merged 15 commits into main from jed/async-closure
Jun 5, 2022

Conversation

@j-f1
Copy link
Member

@j-f1 j-f1 commented Feb 3, 2022 •
edited
Loading

Fixes #157, fixes #156

  • Tests

MaxDesiatov and lin72h reacted with thumbs up emoji

This comment was marked as outdated.

Copy link

Thanks for taking a look into this @j-f1. We are facing a problem related to this. I see it's been some time since the last changes in this pull request. We were wondering what your plans regarding this issue are. Thanks again.

Copy link
Member Author

j-f1 commented Apr 19, 2022

Currently, I think all it’s missing are tests for the new functionality. I’m not sure when I will get around to adding those, but if you would like to open a PR against this PR with tests, I’d be happy to review and merge it in.
khelben5 reacted with thumbs up emoji

Copy link

github-actions bot commented Apr 22, 2022 •
edited
Loading

Time Change: +169ms (0%)

Total Time: 18,389ms

View Unchanged
Test name Duration Change
Serialization/Write JavaScript number directly 403ms -4ms (0%)
Serialization/Write JavaScript string directly 446ms -10ms (2%)
Serialization/Swift Int to JavaScript 5,697ms -4ms (0%)
Serialization/Swift String to JavaScript 6,013ms +127ms (2%)
Object heap/Increment and decrement RC 5,830ms +60ms (1%)

@j-f1 j-f1 force-pushed the jed/async-closure branch from 9045b73 to 888e836 Compare April 30, 2022 16:52
Copy link
Member

Does this still need more tests?

Copy link
Member Author

j-f1 commented May 12, 2022

There’s a test, but most of the new API surface is untested. I’m happy to merge if that’s ok with you, but otherwise I don’t really have any ideas for more tests.

Copy link
Member

@kateinoigakukun would you prefer more test coverage here? If so, how would you cover those missing areas?

@j-f1 j-f1 force-pushed the jed/async-closure branch from 3aac07d to 9b95b98 Compare May 12, 2022 14:03
Copy link
Member

I want more tests around the use of catch and then(success:failure) APIs at least to avoid unintentional API breakage in the future.
MaxDesiatov and j-f1 reacted with thumbs up emoji

@j-f1 j-f1 marked this pull request as ready for review June 4, 2022 13:36
@j-f1 j-f1 requested a review from a team June 4, 2022 13:36
Copy link
Member

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@j-f1 j-f1 merged commit f1ef517 into main Jun 5, 2022
@j-f1 j-f1 deleted the jed/async-closure branch June 5, 2022 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MaxDesiatov MaxDesiatov MaxDesiatov approved these changes

@kateinoigakukun kateinoigakukun kateinoigakukun approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Asynchronous calls in JSClosure JSPromise(resolver:) usage

AltStyle によって変換されたページ (->オリジナル) /