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

Fix behavior with large undo lists #149

Closed
wants to merge 1 commit into from
Closed

Fix behavior with large undo lists #149

wants to merge 1 commit into from

Conversation

SerCeMan
Copy link
Contributor

Hello!

It seems like undo doesn't work with large undo lists, because you always grab first 50 elements of a vector. I have tried "undo" in my application and have got wrong results.

Please, fix me if I didn't understand the idea correctly.

@SerCeMan
Copy link
Contributor Author

Fix works for me

@SerCeMan
Copy link
Contributor Author

And also, the possibility to inject middleware to every handler without encapsulating register-handler will be a great feature. (I need possibility to inject my undo middleware to every handler in dev mode)

@danielcompton
Copy link
Contributor

Hi @SerCeMan, thanks for this! re-frame isn't big on tests in general, but this strikes me as a case where some tests could be helpful. Could you add some that fail on the old behaviour and pass on the new?

As far as injecting handlers into lots of middleware goes, you could make a (def dev-middleware [...] to use. Would that work?

@mike-thompson-day8
Copy link
Contributor

@SerCeMan We started off thinking we needed undoable on all events, and then we realised having undo on everything was a problem.

We have this sort of a case: the user clicks on a button, generating a dispatch, and the event handler initiates a server connection, whose on-success handler initiates a further dispatch, and the associated event hander processes that response. And ... you don't want undoable on all those event handlers. Just the first, user initiated one (clicking on the button), not the intermediate ones. Otherwise your users can undo back into intermediate states, like twirly-thing-showing-while-waiting-for-server-response.

Your millage may vary, of course. Perhaps your app has no intermediate states (generally caused by event handlers doing further dispatches).

BUT, if you do decide you want undoable on all event handlers, then just write/use your own version of re-frame.core/register-handler https://github.com/Day8/re-frame/blob/master/src/re_frame/core.cljs#L46-L50

;; untested
(defn register-undoable-handler         ;; my version which always has undo middleware
  ([id handler]
    (re-frame.core/register-handler id [(undoable)] handler)
  ([id middleware handler]
     (re-frame.core/register-handler id [(undoable) middleware] handler)))   

Then, in your app, use reigster-undoable-handler instead of re-frame.core/register-handler

Thinks for the bug fix!!

@danielcompton
Copy link
Contributor

Don't worry about the tests @SerCeMan, I'm working on them now.

danielcompton added a commit that referenced this pull request Jan 28, 2016
Inspired by #149, this adds tests to cover the change that was made. It
also tests the redo logic.
danielcompton added a commit that referenced this pull request Jan 28, 2016
Inspired by #149, this adds tests to cover the change that was made. It
also tests the redo logic.
@danielcompton
Copy link
Contributor

Thanks for this! I've merged your changes in bb78981.

@SerCeMan
Copy link
Contributor Author

Thanks! About undoable, I use it not only for undo/redo behaviour but also for showing the difference between versions in dev mode, it very convenient for me.

@danielcompton
Copy link
Contributor

You might also find https://github.com/Day8/re-frame/wiki/Debugging convenient for that, it lets you trace individual function calls

danielcompton added a commit that referenced this pull request Jan 28, 2016
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

3 participants