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

Map, Set, WeakMap, WeakSet can't be wrapped successfully #88

Open
HugoDF opened this issue Sep 2, 2023 · 4 comments
Open

Map, Set, WeakMap, WeakSet can't be wrapped successfully #88

HugoDF opened this issue Sep 2, 2023 · 4 comments

Comments

@HugoDF
Copy link
Contributor

HugoDF commented Sep 2, 2023

If this PR gets merged -> #87, the reactivity will not wrap Map, Set, WeakMap, WeakSet (which means they'll be returned without any interception)

Without it being merged I don't think they work properly inside of reactive(), the following throws:

const data = reactive({
  map: new Map()
})
data.map.size

Error in the browser (Firefox): Uncaught TypeError: get size method called on incompatible Proxy

Output in a test with the above code:

TypeError: Method get Map.prototype.size called on incompatible receiver #<Map>
 ❯ Object.get src/reactive.ts:208:29
    206|       if (Reflect.has(depProps, p)) return Reflect.get(depProps, p)
    207|
    208|       const value = Reflect.get(...args)
       |                             ^
    209|       // For any existing dependency collectors that are active, add this
    210|       // property to their observed properties.
@Clonkex
Copy link

Clonkex commented Feb 7, 2024

Can confirm, Set doesn't work as a reactive property 😢

@justin-schroeder
Copy link
Owner

They dont work inside reactive() — and honestly I’m leaning towards not implementing them either. Getting a reactive framework to work well in the ~2kb range is pretty tough and the smaller the api surface area the better. Proxying maps and sets would add some precious bytes to the pile — not ruling it out though. After the refactors ship if there is any room in the byte budget this would be something to look at doing.

@Clonkex
Copy link

Clonkex commented Feb 21, 2024

That's actually ok with me so long as the documentation makes this very clear. I would prefer that it throws a sensible exception as well, but I don't know how much spare room you have for error checking.

@keturn
Copy link

keturn commented Apr 9, 2024

ditto for TypedArray.

My current use case is fine with an immutable TypedArray—no need to react to changes to its individual elements—but it was an unpleasant surprise to discover the thing I retrieved from the state store was not usable.¹

I could work around it if I could unwrap the object from the proxy, but it doesn't look like it exposes its proxySource. Hmm… Reflect.getPrototypeOf(proxy) appears to do that, but that feels more like a lucky implementation detail than a well-defined API.

1: despite the fact that it went in to the store just fine.

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

No branches or pull requests

4 participants