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

Jump to bottom

Add async closure support #159

Merged
merged 15 commits into from
Jun 5, 2022
Merged

Add async closure support #159

merged 15 commits into from
Jun 5, 2022

Conversation

j-f1
Copy link
Member

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

Fixes #157, fixes #156

  • Tests

@github-actions

This comment was marked as outdated.

Sign in to view
@khelben5
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.

@j-f1
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.

@github-actions
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
@MaxDesiatov
Copy link
Contributor

Does this still need more tests?

@j-f1
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.

@MaxDesiatov
Copy link
Contributor

@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
@kateinoigakukun
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.

@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
MaxDesiatov
MaxDesiatov approved these changes Jun 5, 2022
View reviewed changes
Copy link
Contributor

@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
4 participants
@j-f1 @khelben5 @MaxDesiatov @kateinoigakukun