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

Enable promises behind a feature #238

Merged
merged 2 commits into from
Feb 14, 2023
Merged

Enable promises behind a feature #238

merged 2 commits into from
Feb 14, 2023

Conversation

jeffcharles
Copy link
Collaborator

@jeffcharles jeffcharles commented Feb 14, 2023

The feature is off by default. It can be enabled by running make cli CORE_FEATURES=experimental_event_loop and tests can be run by invoking make test-cli CORE_FEATURES=experimental_event_loop CLI_FEATURES=experimental_event_loop.

Copy link
Collaborator

@surma surma left a comment

Choose a reason for hiding this comment

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

Nice!

@surma
Copy link
Collaborator

surma commented Feb 14, 2023

Simplified the test runner since we can now rely on proises :D

@saulecabrera
Copy link
Member

I left an offline note to @jeffcharles, but I'll add here too for visibility: before landing this I'd like for us to ensure that unhandled rejected promises are behaving correctly, a preliminary test on my end shows that the current behaviour doesn't throw, which is wrong afaik. So we might have to fix that.

@surma
Copy link
Collaborator

surma commented Feb 14, 2023

This is probably also a reminder that we should run test262

@saulecabrera
Copy link
Member

Yeah, going a bit further, wpt has a suite for promises https://web-platform-tests.org/writing-tests/testharness-api.html#promise-rejection, so perhaps we should enable that to avoid missing any edge cases.

@jeffcharles
Copy link
Collaborator Author

Discussed offline, we're going to put this behind a default off feature for now

@jeffcharles jeffcharles changed the title Enable synchronous promises Enable promises behind a feature Feb 14, 2023
Copy link
Collaborator

@surma surma left a comment

Choose a reason for hiding this comment

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

🎉

@saulecabrera saulecabrera enabled auto-merge (squash) February 14, 2023 18:04
@saulecabrera saulecabrera merged commit ec6ebc0 into main Feb 14, 2023
@saulecabrera saulecabrera deleted the jc.flush-pending-jobs branch February 14, 2023 18:29
saulecabrera added a commit to saulecabrera/javy that referenced this pull request Jul 8, 2024
This commit refactors the event loop handling code, mainly by extracting
the common pieces into a reusable function.

Additionally, this commit ensures that the result of `Promise.finish` is
corectly handled, which fixes execution of code with top level awaits.

Finally, this commit also ensures that we test the
`experimental_event_loop` in CI for the CLI and core crates (follow-up
to bytecodealliance#238)
@saulecabrera saulecabrera mentioned this pull request Jul 8, 2024
3 tasks
saulecabrera added a commit that referenced this pull request Jul 8, 2024
* Tidy up event loop handling code

This commit refactors the event loop handling code, mainly by extracting
the common pieces into a reusable function.

Additionally, this commit ensures that the result of `Promise.finish` is
corectly handled, which fixes execution of code with top level awaits.

Finally, this commit also ensures that we test the
`experimental_event_loop` in CI for the CLI and core crates (follow-up
to #238)

* Clippy fixes

* Assert fuel conditionally

When the `experimental_event_loop` feature is enabled, a bit more work
is done when executing JS code, therefore there's more fuel usage
reported in some tests.

* Create `cli-features.yml`

* Add right naming

* Fix `runs-on` key

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

Successfully merging this pull request may close these issues.

3 participants