Skip to content

Implement RandomAccessCollection on JSArrayRef - #30

Merged
kateinoigakukun merged 2 commits into
masterfrom
random-access-collection
Aug 4, 2020
Merged

Implement RandomAccessCollection on JSArrayRef#30
kateinoigakukun merged 2 commits into
masterfrom
random-access-collection

Conversation

@MaxDesiatov

@MaxDesiatov MaxDesiatov commented Aug 4, 2020

Copy link
Copy Markdown
Member

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.

@j-f1 j-f1 left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member 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