-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat(devtools): state diffing #1585
base: v2
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for pinia-official canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty neat!
Make sure to run the lint:fix script in your files
Can you should some screenshots of the added features?
@@ -328,6 +328,8 @@ function addStoreToDevtools(app: DevtoolsApp, store: StoreGeneric) { | |||
store.$onAction(({ after, onError, name, args }) => { | |||
const groupId = runningActionId++ | |||
|
|||
const initialState = JSON.parse(JSON.stringify(toRaw(store.$state))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably try to use https://developer.mozilla.org/en-US/docs/Web/API/structuredClone if it exists.
It should definitely gracefully fail for circular references and not crash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you doing toRaw()
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toRaw
helps with cloning the object. I've tested structuredClone
function but it seems it has problems with cloning functions.
I can create my own deepClone function or use the one that lodash provides, but I don't know if you want to have dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there shouldn't be functions in state 😓
Yes, we want to avoid importing lodash
structuredClone have some limitations but it should also be quite fast. You should probably avoid doing toRaw()
because we want to unwrap the properties.
I remember Vue devtools had a function for exactly this usecas in their codebase, it might be worth checking it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, because state is wrapped in a proxy with methods, if I don't make this a raw object structuredClone
won't work.
JSON.parse(JSON.stringify)
will work without toRaw
but the effect will be I think the same.
Could you explain to me why toRaw
should be avoided in this situation?
I use this raw
object just to copy and display it in devtools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of nested refs
@posva I've added fast-copy util which is also used in vue devtools for vuex state copy. As far as I could see it all works great. I've attached some images of devtools. |
Where is fast-clone used in devtools? |
Hi inspired by redux devtools and also this issue: #859
I've decided to add more debug information to pinia devtools about the status of the state before and after given action.
In acknowledgment that states can be pretty huge I've also decided to add
differences
object that only holds data that has been changed.I think it works pretty well, but it will crash for any circular objects. On the other hand I don't think that those kinds of objects should stored in pinia state.
Let me know what you think :)
Here are some images of new devtools feedback: