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

Code comments / architecture explanation #171

Closed
akutruff opened this issue Jul 8, 2021 · 13 comments
Closed

Code comments / architecture explanation #171

akutruff opened this issue Jul 8, 2021 · 13 comments

Comments

@akutruff
Copy link

akutruff commented Jul 8, 2021

Code comments and a high level design doc would be really appreciated. I'm trying to grock what's going on and there's quite a bit of code in each function / it's unclear what's happening in each layer.

For example:

export const useSnapshot = <T extends object>(

My eyes are crossing trying to drill through this even before getting into the vanilla layer.

I'd start with useMutableSource and how it is used. What's the snapshot supposed to be. I know you're emulating the new API in React, but I can't make out what's actually in a snapshot. It's a proxy? When if ever is data copied and at what layer?

@dai-shi
Copy link
Member

dai-shi commented Jul 8, 2021

valtio has two kinds of proxies, for write and read. We intentionally separate them for hooks and concurrent react.

proxy() creates a proxy object to detect mutation, aka set handler.
snapshot() creates an immutable object from the proxy object.
useSnapshot() wraps the snapshot object again with another proxy (with proxy-compare) to detect property access, aka get handler.

both kinds of proxies create nested proxies on demand (and that's a tough task, which makes code hard.)

I guess you want to start from vanilla.ts.

@akutruff
Copy link
Author

akutruff commented Jul 9, 2021

Thanks for that explanation. It's a good start and I encourage you to put that in a document or in the code comments and expand a lot more. It's your project of course, and if you feel the code is clear enough feel free to close this issue.

@dai-shi
Copy link
Member

dai-shi commented Jul 9, 2021

Let me add some code comments and hope to get some feedback from you. I would say the code is not very readable without high level abstraction.

@dai-shi
Copy link
Member

dai-shi commented Jul 14, 2021

Although I'm not sure how much is covered, #174 adds something.

@akutruff
Copy link
Author

I'd add interaction and data flows to a .md file or in the comments. The change you put in are general details but for a person who doesn't know the code won't get much from. Since you're not having high level abstraction in the actual code, you can provide that abstraction through comments. Pretend you have no idea what anything is when documenting the code as a newbie like myself won't be able to connect the dots.

  1. Starting with useSnapshot() what proxies are created and what are their conceptual names? Briefly, how is useMutableSource() source used to setup the subscription? It looks like there are listeners on a proxy, for deep property changes, how are those stored and tracked? What will trigger the subscription to be fired? Be detailed here.
  2. Walk through the flow of a property change for something non-trivial like address.person.name = "Bob" Step-by-step, from the moment of the assignment, what happens? How is the change notification queued? Is it through a React hook? When the change notification actually occurs, how is a re-render triggered? Leave no stone unturned as this is a very complicated interaction.

@dai-shi
Copy link
Member

dai-shi commented Jul 14, 2021

Hmm, it seems to me like code comments are not good for this goal, at least didn't work for me. Let's create a wiki page so that you can also work on it, like adding questions.

Here we go: https://github.com/pmndrs/valtio/wiki/How-valtio-works

@zcaudate
Copy link

+1 to the wiki page and for diagrams on how proxies update.

It'd also be great to to get some comparisons with other libraries like jotai (comparing how the internals are implemented)

@dai-shi
Copy link
Member

dai-shi commented Jul 15, 2021

@akutruff 's comment in #155 (comment) :

Snapshotting the whole thing is not an option if you have lots of objects.

I think we should add this in the wiki page, but let me try describing how snapshot creation is optimized in valtio.

const state = proxy({ a: { aa: 1 }, b: { bb: 2 } })
const snap1 = snapshot(state)
console.log(snap1) // ---> { a: { aa: 1 }, b: { bb: 2 } }
++state.a.aa
const snap2 = snapshot(state)
console.log(snap2) // ---> { a: { aa: 2 }, b: { bb: 2 } }
snap1.b === snap2.b // this is `true`, it doesn't create a new snapshot because no properties are changed.

@zcaudate
Copy link

@dai-shi: is it possible to add a subscribeDiffs function to a proxy?

@dai-shi
Copy link
Member

dai-shi commented Jul 16, 2021

I'm not sure what you mean by subscribeDiffs, but check how subscribeKey is implemented, and you would know you could do something similar.

@dai-shi
Copy link
Member

dai-shi commented Dec 26, 2021

Updated: https://github.com/pmndrs/valtio/wiki/How-valtio-works
Finished my action items I planed, so closing this.
If anyone has suggestions or even contributions, they are welcome.

@dai-shi dai-shi closed this as completed Dec 26, 2021
@sambacha
Copy link

Updated: pmndrs/valtio/wiki/How-valtio-works Finished my action items I planed, so closing this. If anyone has suggestions or even contributions, they are welcome.

To let you know, search engines are unable to index github wiki's, so all the content in the wiki is not available to be searched

@dai-shi
Copy link
Member

dai-shi commented Dec 28, 2021

Ah, thanks for the note. that is a drawback, compared to putting the content in the repo.

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