Skip to content

Implement RandomAccessCollection on JSArrayRef #30

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

Merged
merged 2 commits into from
Aug 4, 2020

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Aug 4, 2020

This allows using Int subscripts on JSArrayRef instances and makes the rest of the functions available from the relevant extensions. This may shadow some JavaScript functions with the same names (I don't remember any off the top of my head), but I hope that the added convenience is worth it.

Also .gitignore is amended to include the .build directory created by make test.

Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

Great! The reason I went so far with the JSObjectRef is because an object typically has a bunch of properties and I wanted them to be accessible. Since arrays usually don’t have additional properties, I think that in the rare case there’s a conflict, it’s ok to have to use the subscript syntax.

@MaxDesiatov
Copy link
Contributor Author

After having another look, JSArrayRef doesn't have dynamic member subscripts, so none of the JS properties or functions are exposed on it, no chance of collision then. If a user needs to access those they should just cast that object to JSObjectRef through the .object property of JSValue instead of .array.

@MaxDesiatov MaxDesiatov added the enhancement New feature or request label Aug 4, 2020
@kateinoigakukun kateinoigakukun merged commit 19365a1 into master Aug 4, 2020
@kateinoigakukun kateinoigakukun deleted the random-access-collection branch August 4, 2020 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants