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

state.undo / redo does not seem to work #27

Open
pedroAquino opened this issue Jun 20, 2022 · 8 comments
Open

state.undo / redo does not seem to work #27

pedroAquino opened this issue Jun 20, 2022 · 8 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@pedroAquino
Copy link

pedroAquino commented Jun 20, 2022

Thanks for the awesome library,

I'm trying to make the undo / redo from valtio's proxyWithHistory work but it seems that it is not triggering yjs to update on undo / redo.

Code sandbox example here:

https://codesandbox.io/s/valtio-history-counter-d6txif?file=/src/App.js:329-345

any ideas on how to solve this ?

@dai-shi
Copy link
Member

dai-shi commented Jun 20, 2022

Hey, thanks for trying the library.
Confirmed the behavior, but I don't know how it's happening.
We would need a test to investigate further and keep the feature for the future.
Can you try to reproduce the issue minimally without proxyWithHistory? (You may need to read the code.)

@dai-shi dai-shi added bug Something isn't working help wanted Extra attention is needed labels Jun 20, 2022
@pedroAquino
Copy link
Author

pedroAquino commented Jun 20, 2022

Hey @dai-shi thanks for the quick response

Can you try to reproduce the issue minimally without proxyWithHistory? (You may need to read the code.)

what did you mean by that ? I can't call undo / redo without proxyWithHistory correct ?

@dai-shi
Copy link
Member

dai-shi commented Jun 20, 2022

Yeah, I mean to ask help for debugging.
proxyWithHistory is just a util around proxy. So, the bug can be revealed by reproducing it only with proxy at minimum.
If you are not familiar with the implementation, just ignore...

@pedroAquino
Copy link
Author

I'm somehow familiar with the implementation of valtio-yjs but not valtio itself, will do some debugging later this week 👍

@vezwork
Copy link

vezwork commented Jun 15, 2023

I'm interested in using valtio-yjs with either proxyWithHistory or y.js' undoManager. Has there been progress on this bug? I tried adding UndoManager usage to this valtio-yjs demo but calling undo did not change the data at all.

@dai-shi
Copy link
Member

dai-shi commented Jun 15, 2023

At the moment, I'd say valtio-yjs and proxyWithHistory conflict. (Help wanted)
I would avoid using proxyWithHistory (which is just a tiny util) for the moment, and try handmade undo feature.

@PhilGarb
Copy link

@vezwork when I had issues with the yjs undoManager it helped to assign a transaction origin to the undo manager and valtio-yjs.

const undoManager = new Y.UndoManager(yMap, {
  trackedOrigins: new Set(["valtio"]),
});

bind(syncedStore, yMap, {
  transactionOrigin: "valtio",
});

I am not quite sure why this is necessary, but after adding it it worked for me.

@vezwork
Copy link

vezwork commented Jun 16, 2023

Thanks @PhilGarb, I just tried that but undo still does not seem to do anything. Here is what I was trying: https://codesandbox.io/s/valtio-yjs-demo-forked-2hzskt?file=/src/App.js

edit: Never mind! It works! I upgraded the example's version of valtio-yjs to 0.5.0 from 0.1.0, and used bind instead of bindProxyAndYmap and it now works :). Thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants