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

feat(devtools): state diffing #1585

Open
wants to merge 6 commits into
base: v2
Choose a base branch
from

Conversation

KrosFire
Copy link

@KrosFire KrosFire commented Aug 23, 2022

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: Screenshot 2022-08-24 at 17 37 56 Screenshot 2022-08-24 at 17 38 12 Screenshot 2022-08-24 at 17 38 25 Screenshot 2022-08-24 at 17 38 39

@netlify
Copy link

netlify bot commented Aug 23, 2022

Deploy Preview for pinia-official canceled.

Name Link
🔨 Latest commit c19379d
🔍 Latest deploy log https://app.netlify.com/sites/pinia-official/deploys/63067b8837cd10000816c6f2

@posva posva changed the title Added functionality for checking differences in state feat(devtools): state diffing Aug 24, 2022
Copy link
Member

@posva posva left a 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)));
Copy link
Member

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

Copy link
Member

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?

Copy link
Author

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

Copy link
Member

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

Copy link
Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Because of nested refs

@KrosFire KrosFire requested a review from posva August 24, 2022 19:29
@KrosFire
Copy link
Author

@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.

@posva
Copy link
Member

posva commented Oct 6, 2022

Where is fast-clone used in devtools?
I think we might want to go a little bit more complicated with partial state patch objects so let's hold onto this feature for the moment

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.

None yet

2 participants