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

Array Indexing #37

Closed
wants to merge 3 commits into from
Closed

Array Indexing #37

wants to merge 3 commits into from

Conversation

benpate
Copy link
Contributor

@benpate benpate commented Feb 20, 2021

This PR adds the array index operators so that hyperscript programs can not look up array values using this notation: "array[index]"

There is more work to do on this, but it would be good to have another set of eyes on this first, before I try making any more additions to hyperscript.

- This PR adds the array index operators so that hyperscript programs can not look up array values using this notation: "array[index]"
@benpate benpate changed the title Adding array indexing Array Indexing Feb 20, 2021
Copy link
Contributor

@1cg 1cg left a comment

Choose a reason for hiding this comment

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

Really good first effort! Impressed that you managed to make any sense of my code!

return _runtime.unifiedEval(this, function(context, rootVal){
return rootVal[index];
}, context, root)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Close! The trick is to pass both root and index in to a single unifiedEval:

return _runtime.unifiedEval(this, function(context, rootVal, index){
                                    return rootVal[index];
                                }, context, root, indexExp)

It isn't documented at all, but the arguments that are passed in after the function are correctly resolved and then passed back to that function, in order. So you get context, root and indexExp all evaluated async-transparently and handed back to you.

Also, I don't think we need to parse an int here, we can rely on the user to supply the correct number type.

_parser.addGrammarElement("arrayIndex", function (parser, tokens, root) {

if (tokens.matchOpToken("[")) {
var indexExp = parser.parseElement("expression", tokens)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have introduced a new function requireElement that you probably want to use here, so it throws a parse error if no element is returned.

Now that I understand `_runtime.unifiedEval` a little better, I'm able to post this cleaner (probably more efficient) code.

- Single call to `_runtime.unifiedEval` instead of two.
- Clearer use of `op` function call should be easier to follow, now.
- All tests passing
@benpate
Copy link
Contributor Author

benpate commented Feb 22, 2021

Updating this because I hadn't read all of your comments before I posted my update. I don't see requireElement in my branch. Was that pushed to upstream/functions or do I need to pull it from somewhere else?

@1cg
Copy link
Contributor

1cg commented Feb 27, 2021

I merged this manually to port to the new runtime, thank you!

@1cg 1cg closed this Feb 27, 2021
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.

None yet

2 participants