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 Array.prototype.flatMap, Array.prototype.flat #5543

Closed
keithamus opened this issue Jul 27, 2018 · 7 comments · Fixed by #5573
Closed

Implement Array.prototype.flatMap, Array.prototype.flat #5543

keithamus opened this issue Jul 27, 2018 · 7 comments · Fixed by #5573

Comments

@keithamus
Copy link

flatMap and flat are now Stage 3. https://github.com/tc39/proposal-flatMap

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jul 27, 2018

I'd be keen on implementing this if it's not already planned for. Please let me know if an external contribution would be welcome here.

@digitalinfinity
Copy link
Contributor

Go for it! I would expect these can be written as JS built-ins? @meg-gupta might have pointers here.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jul 28, 2018

OK - I'll make a start and see how I get on, I think that for CC mechanics I can follow the model used for Array.prototype.filter so hopefully won't need much assistance. I'll also endeavour to test against all of the relevant test-262 tests before submitting anything.

@meg-gupta
Copy link
Contributor

ChakraCore/lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js - has our builtins written in Javascript. Array.prototype.indexOf is also a good implementation to follow via the Js route.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Aug 4, 2018

One question I could do with help with, implementing a ethodwithin JsBuiltIn.js is it possible to have another JavaScript Function that it calls?

Motivation is that I cannot see a good way to .flat or .flatMap without using the FlattenIntoArray function described in the spec - I've written this function in JavaScript but for some reason including this inside JsBuiltIn.js and then trying to call it from within my Array.Flat implementation results in a hard crash at runtime - is there anything obvious I'm missing?

If I put the below into JsBuiltIn.js, regenerate bytecode, build CC and then call the Array.prototype.flat method at runtime I get a hard crash:

    const FlattenIntoArray = function ( ) {}

    platform.registerFunction(FunctionsEnum.ArrayFlat, function (depth) {
        "use strict";
        FlattenIntoArray(); // hard crash
    }

Deleting the FlattenIntoArray() call results in the code running to completion.

@meg-gupta
Copy link
Contributor

A good example for such a helper function would be CreateArrayIterator in JsBuiltins.js. We register them via registerChakraLibraryFunction and which adds them to the __chakraLibrary object. Functions on __chakraLibrary will be treated internal and will not be visible outside. Unfortunately we have not added documentation on the process of adding a Js builtin, I'll try to make that happen soon.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Aug 4, 2018

Thanks for the help, it all seems to be working now, I'll sort out some appropriate tests then submit it.

chakrabot pushed a commit that referenced this issue Aug 17, 2018
Merge pull request #5573 from rhuanjl:flat

This PR implements the stage 3 proposal Array methods: Array.prototype.flat and Array.prototype.flatMap as JsBuiltIns.

Draft specification text is here: https://tc39.github.io/proposal-flatMap/

Notes:
1. 3 test262 failures:
     - length is not configurable (for both methods), this is due to ScriptFunction lengths in CC all not being configurable and should be fixed if #5405 is merged
    - the case using null as this fails -- EDIT: FIXED
2. I can't find a feasible way to test the length is too big error - not sure if CC can actually handle arrays that big anyway so maybe not worth implementing that case?
3. Should probably have a few more CI tests

Fixes #5543
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants