Skip to content

Conversation

@chrfalch
Copy link
Collaborator

@chrfalch chrfalch commented Apr 6, 2024

Fixed proxying so that shared values uses proxies, while all other wrapping doesn't. This solves issues described in #158

Closes #158

chrfalch and others added 14 commits April 6, 2024 11:49
This one:

```js
const sharedArray = Worklets.createSharedValue([0, 1, 2, 3]);
const isArray = Array.isArray(sharedArray.value); // <- returns false
```

Is now working.
To be able to perform tests and verify if an object is a valid jsi::Array and jsi::Object, a couple of "hidden" methods has been added: __jsi_is_array and __jsi_is_object.
A lot of the classes had misleading ctors, like accepting runtime/value parameters without using them.

This cleans up this mess.
This will help us decide if we should create a copy or a wrapper when unwrapping later.
Now working so that values created with createSharedValue will return proxies when needed, while wrappers other than this returns a copy of arrays.
@hannojg hannojg requested a review from mrousavy April 8, 2024 12:02
@mrousavy
Copy link
Member

mrousavy commented Apr 8, 2024

Is that PR ready?

@chrfalch
Copy link
Collaborator Author

chrfalch commented Apr 8, 2024 via email

@chrfalch chrfalch changed the title Fix/158 is array fix: Make sure isArray passes correctly Apr 9, 2024
Copy link
Member

@mrousavy mrousavy left a comment

Choose a reason for hiding this comment

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

Looks great, really nice! 💪

Comment on lines +468 to +469
" if (prop === 'length') return "
"Object.keys(target).length;"
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this now changed behaviour? Doesn't .length now include all functions, like .map(), etc as well?

@mrousavy mrousavy merged commit 5d55718 into main Apr 9, 2024
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.

Shared values with arrays are not recognised by JS Array.isArray() as valid arrays

3 participants