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

Normative: Use onFinally's realm when creating functions in Promise.prototype.finally (fixes #2222) #2233

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arai-a
Copy link
Contributor

@arai-a arai-a commented Nov 24, 2020

Changed Promise.prototype.finally to use onFinally's realm when creating thenFinally and catchFinally,
so that embedding uses onFinally's realm's global when checking if job's global is fully active.

This fixes #2222

@ljharb ljharb added needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Nov 24, 2020
@ljharb
Copy link
Member

ljharb commented Nov 24, 2020

Does this need consensus, and/or does it match what engines already do? My assumption is yes and yes.

@arai-a
Copy link
Contributor Author

arai-a commented Nov 24, 2020

test is in tc39/test262#2910.
According to the result there, most engines follow current spec (create thenFinally and catchFinally in finally method's realm) and don't match to the modified behavior.
As described in issue #2222, this mostly affects only embedding scenario, and this is normative change for consistency.

I don't know about consensus requirement, but would be nice to get feedback from devs?

@ljharb ljharb added the needs consensus This needs committee consensus before it can be eligible to be merged. label Nov 24, 2020
@ljharb
Copy link
Member

ljharb commented Nov 24, 2020

Seems best to discuss it in the next plenary.

The “has tests” label will be applied when the tests PR is merged, after it has consensus.

@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
@ljharb
Copy link
Member

ljharb commented Nov 17, 2021

I've added this to the agenda for next month's meeting.

@ljharb
Copy link
Member

ljharb commented Mar 31, 2022

This was discussed (1 2).

Conclusion was that @codehag would update the PR/issue to refer to Mozilla's bug, and for @codehag, @erights, and myself to discuss further, which we have not yet done.

@codehag
Copy link
Contributor

codehag commented Apr 1, 2022

Thanks for the reminder, I will find some time for this before the next meeting.

@ljharb
Copy link
Member

ljharb commented Feb 20, 2023

ping @codehag @dminor, it'd be great to get this moving forward, if it's still important.

@codehag
Copy link
Contributor

codehag commented Feb 20, 2023

We still want to do this. I just need to refresh my memory on it, thanks again for ping.

@ljharb
Copy link
Member

ljharb commented Jul 28, 2023

@dminor any update from spidermonkey on this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Promise.prototype.finally should use onFinally's realm for newly creating functions
3 participants