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

Implement Promise.prototype.finally #3520

Closed
ljharb opened this issue Aug 12, 2017 · 13 comments
Closed

Implement Promise.prototype.finally #3520

ljharb opened this issue Aug 12, 2017 · 13 comments

Comments

@ljharb
Copy link
Collaborator

ljharb commented Aug 12, 2017

Promise.prototype.finally is stage 3 - time to implement it! :-D

Test262 tests are merged.

Spec is here.

@ljharb
Copy link
Collaborator Author

ljharb commented Sep 28, 2017

This is stage 3, and I'm hoping to seek stage 4 in November or January; any update on this?

@dilijev
Copy link
Contributor

dilijev commented Sep 29, 2017

This work is tentatively planned for the near future if we can fit it in. Earliest we can begin work on it is late October / early November.

@ljharb
Copy link
Collaborator Author

ljharb commented Nov 9, 2017

@dilijev early November is here!

@dilijev
Copy link
Contributor

dilijev commented Nov 10, 2017

@ljharb Unfortunately, this didn't make the cut for our feature work on the upcoming release. We'll re-evaluate priorities and see when we can tackle this work item.

@ljharb
Copy link
Collaborator Author

ljharb commented Nov 10, 2017

@dilijev please note that I'm seeking stage 4 for it in 3 weeks; if it doesn't make it then, I'd expect it to do so in January.

@dilijev
Copy link
Contributor

dilijev commented Nov 10, 2017

@ljharb at this point, it will not make it into the next Windows/Edge release. Best case the feature could be checked in to ChakraCore master and/or Node-ChakraCore, but we can't commit to anything at the moment. What is the essence of the implemented feature requirement for the process?

@ljharb
Copy link
Collaborator Author

ljharb commented Nov 11, 2017

It already has the requisite two (three, now) implementations; i was just hoping it could hit stage 4 with all 4 browsers - as is, it seems like edge will be the last to implement it.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jan 29, 2018

Is anyone working on this? If not I'd like to have a stab at making a PR for it.

@ljharb
Copy link
Collaborator Author

ljharb commented Jan 29, 2018

This is now stage 4, so it'd be great to get it implemented ASAP!

@dilijev
Copy link
Contributor

dilijev commented Jan 30, 2018

@rhuanjl AFAIK no one is currently working on this. Feel free to take a stab at it.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jan 30, 2018

OK, will take me at least a week, maybe two, I'll use this comment as a tracker and post here if for any reason I don't think I can do it, please let me know at any point if it's being picked up internally (i.e. if I should drop it).
Todo prior to opening a PR:

  • have ChakraCore recognise Promise.prototype.finally
  • implement function behind it
  • test using test262 test cases
  • revise as necessary until all tests passed
  • add appropriate test cases to CC CI tests

@dilijev
Copy link
Contributor

dilijev commented Jan 30, 2018

@rhuanjl test262 test cases may fail for reasons unrelated to this work (we unfortunately have thousands of test262 failures that we're tracking, probably not thousands of individual root causes). Use your best judgment but you don't have to block on test262.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jan 30, 2018

dilijev thanks for the heads up I'll do the best I can for spec conformance and passing the relevant tests.

chakrabot pushed a commit that referenced this issue Feb 15, 2018
Merge pull request #4668 from rhuanjl:experiment

This PR implements Promise.prototype.finally.

Fixes #3520
Relevant ECMASpec: https://tc39.github.io/ecma262/#sec-promise.prototype.finally

**Notes:**
1. I worked out how to do this by reading the code for Promise.prototype.catch and the internal AsyncSpawnExecutorFunction - there is a chance that some of what I've done isn't quite right though it does all seem to work.
2. I've run this against the relevant testcases from test262 and it passes them.
3. It probably could do with a few more CI tests - I added what seemed good to me based on what was there for other promise methods.
4. The large size of the diff is as I had to regenerate the bytecode for built in methods after adding the entry point for finally to JnDirectFields.h

**cc:** @dilijev @ljharb

Thanks to my friend @fatcerberus for running the RegenerateBytecode script for me.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants