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

Frozen page when proxying an instance of EventEmitter #178

Closed
StarpTech opened this issue Jul 18, 2021 · 23 comments · Fixed by #180
Closed

Frozen page when proxying an instance of EventEmitter #178

StarpTech opened this issue Jul 18, 2021 · 23 comments · Fixed by #180

Comments

@StarpTech
Copy link

StarpTech commented Jul 18, 2021

From #62 (comment)

The following code will result in:

Uncaught TypeError: 'set' on proxy: trap returned falsish for property 'Notification'
import EventEmitter from 'events';
import { proxy } from 'valtio';

export const state = proxy({
  signals: new EventEmitter()
});

// anywhere
const { signals } = useSnapshot(state);
useEffect(() => {
 signals.on('Notification', fn);
 return () => {
   return signals.off('Notification', fn);
 };
}, [signals)

Workaround

Avoid proxying EventEmitter with ref()

import { proxy, ref } from 'valtio';

export const state = proxy({
  isConnecting: false,
  signals: ref(new EventEmitter()),
  connection: null
});

I'd like to understand why it fails so badly. I'd assume that the proxy handles it transparently.

@dai-shi
Copy link
Member

dai-shi commented Jul 18, 2021

snapshot() returns a pure object.
useSnapshot() returns a wrapped snapshot object with another proxy for render optimization.
This is handled by proxy-compare. As snapshot is a frozen object, set is prohibited.

Object.freeze(snapshot)

https://github.com/dai-shi/proxy-compare/blob/015f3291dbdf5bccd9dd22c095afaf13febd4981/src/index.ts#L116-L118

ref() is to cancel this behavior, so it's a proper usage in the case of EventEmitter.

I'd assume that the proxy handles it transparently.

EventEmitter wouldn't work even without snapshot. proxy() creates a new tree from the initial object (recursively applying proxy()) and coping EventEmitter might be troublesome/impossible. To avoid this, ref() is introduced.

refSet.has(value) ||

@StarpTech
Copy link
Author

StarpTech commented Jul 19, 2021

Thanks for the explanation! Is there a chance to throw a more meaningful error in that case?

@dai-shi
Copy link
Member

dai-shi commented Jul 19, 2021

It would be nice but pretty difficult. The meaningful message could only be made at creation.
The safe bet is only use serializable objects for proxy() and wrap non-serializable objects with ref().

@StarpTech
Copy link
Author

Do you know why EventEmitter is special and what do you mean by serializable in this context?

@dai-shi
Copy link
Member

dai-shi commented Jul 19, 2021

No. What would happen if you do this?

p = proxy(new EventEmitter())
p.on('Notification', fn)

By serializable, I almost meant JSON.serialize-able.

@dai-shi
Copy link
Member

dai-shi commented Jul 19, 2021

> new EventEmitter().domain
Domain {
  _events: [Object: null prototype] {
    removeListener: [Function: updateExceptionCapture],
    newListener: [Function: updateExceptionCapture],
    error: [Function: debugDomainError]
  },
  _eventsCount: 3,
  _maxListeners: undefined,
  members: [],
  [Symbol(kCapture)]: false,
  [Symbol(kWeak)]: WeakReference {}
}

null prototype looks unusual. WeakReference is not properly excluded. Not sure if this is the fundamental reason. There could be so many of them.

@StarpTech
Copy link
Author

StarpTech commented Jul 19, 2021

p = proxy(new EventEmitter())
p.on('Notification', fn)

produce no error.

@StarpTech
Copy link
Author

StarpTech commented Jul 19, 2021

The trigger is useSnapshot. It will throw in that isolated test with TypeError: Invalid value used as weak map key.

  93 | const getMutableSource = (proxyObject) => {
  94 |   if (!mutableSourceCache.has(proxyObject)) {
> 95 |     mutableSourceCache.set(proxyObject, createMutableSource(proxyObject, getVersion));
  96 |   }
  97 |   return mutableSourceCache.get(proxyObject);
  98 | };

@StarpTech
Copy link
Author

null prototype looks unusual

This is pretty common to not inherit from the default Object prototype. Object.create(null).

@dai-shi
Copy link
Member

dai-shi commented Jul 19, 2021

Oh, so you don't have any issues unless you use useSnapshot?

It will throw in that isolated test with TypeError: Invalid value used as weak map key.

This means the proxyObject is not an object.

This is pretty common to not inherit from the default Object prototype. Object.create(null).

Yeah, maybe there's no issue on this.

@StarpTech
Copy link
Author

Oh, so you don't have any issues unless you use useSnapshot?

Yes

@dai-shi
Copy link
Member

dai-shi commented Jul 20, 2021

Okay, so proxy-compare is the library used in useSnapshot for tracking state usage (= property access).
It basically supports plain objects.
https://github.com/dai-shi/proxy-compare/blob/015f3291dbdf5bccd9dd22c095afaf13febd4981/src/index.ts#L19-L25

However, in valtio, we found there's a good use case to define a state with class syntax.
So, to bypass the check in proxy-compare, we explicitly support object with prototype.

markToTrack(snapshot, true) // mark to track

However, this caused a problem when people put non plain objects like EventEmitter.
So, we discussed in #61, and took opt-out approach with ref().

markToTrack(value, false) // mark not to track

And, these exceptions confused you...

So, in summary, valtio supports plain objects and arrays, plus simple class syntax to produce plain objects,
and non-plain objects should be wrapped with ref().

@StarpTech
Copy link
Author

From the developer's perspective, this is highly confusing because I assumed that I can assign any js value. I'd added it to the README.

@dai-shi
Copy link
Member

dai-shi commented Jul 20, 2021

Yeah, let's do that.

@StarpTech
Copy link
Author

StarpTech commented Jul 20, 2021

Don't you think that splitting state from none-serializable data like functions & Co would be a better design? In that way, we can throw meaningful errors and the separation is clear.

import { proxy } from 'valtio'

export const state = proxy({
  count: 0,
  name: 'foo',
})

export const actions = {
  inc: () => {
    ++state.count
  },
  setName: (name) => {
    state.name = name
  },
}

Disallowed (Only objects and arrays allowed)

export const state = proxy({
  count: 0,
  name: 'foo',
  inc() {
    ++this.count
  },
  setName(name) {
    this.name = name
  },
})

@dai-shi
Copy link
Member

dai-shi commented Jul 20, 2021

We accept both patterns: https://github.com/pmndrs/valtio/wiki/How-to-organize-actions
(I prefer the separation, but the lib allows both.)

So, "serializable-only" was a wrong conception.
I'm still wondering how to note this correctly and nicely.

Do you know why EventEmitter is special

I'd say plain objects (no custom prototype) or user-defined prototype (class syntax) are fine.
But, it sounds too technical and is not well said from the developer's perspective.

@dai-shi
Copy link
Member

dai-shi commented Jul 22, 2021

Not sure if #180 is something you expected, but for now I would like to modify the section about ref(). Let me know if you have suggestion modifying words there.

@dai-shi
Copy link
Member

dai-shi commented Jul 23, 2021

Oh, so you don't have any issues unless you use useSnapshot?

Yes

Actually, I got a clear explanation on this.
Here's the repro.

const state = proxy({
  count: 0,
  inc() {
    ++this.count;
  }
});

console.log("before", state.count);
state.inc(); // this works fine
console.log("after", state.count);

const snap = snapshot(state);
console.log("before", snap.count);
snap.inc(); // this doesn't work because it's trying to mutate frozen snapshot
console.log("after", snap.count);

https://codesandbox.io/s/adoring-lehmann-yji31?file=/src/App.js

@StarpTech
Copy link
Author

ok, this makes this way invalid https://github.com/pmndrs/valtio/wiki/How-to-organize-actions#using-class?

@dai-shi
Copy link
Member

dai-shi commented Jul 23, 2021

https://github.com/pmndrs/valtio/wiki/How-to-organize-actions#action-methods-using-this is also using this.

Both cases are valid as long as we use state.inc(). We send "increment" command to "state".

Both cases are invalid when we do snap.inc(). We can't modify "snapshot".

@StarpTech
Copy link
Author

I got it. The design is very hard to grasp. I'd expect an error like "Snapshot can't be modified, please use the state directly".

Actually, I got a clear explanation on this.
Here's the repro.

IMO this doesn't explain why eventEmitter doesn't work.

@dai-shi
Copy link
Member

dai-shi commented Jul 23, 2021

The design is very hard to grasp.

My recommendation for beginners is no this, and no prototype (aka class).

I'd expect an error like "Snapshot can't be modified, please use the state directly".

That would be nice. At JS level, it would be hard. Maybe a good challenge for eslint-plugin-valtio.

IMO this doesn't explain why eventEmitter doesn't work.

I don't know, but it's the only the reason I can think why state.signals.on('Notification', fn) works but snap.signals.on('Notification', fn) doesn't.

@dai-shi
Copy link
Member

dai-shi commented Jul 29, 2021

I wish I could get more feedback from anyone, but let me move forward.
This will be closed.
We can continue discussion here, in #171 or new discussion.

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 a pull request may close this issue.

2 participants