8000 add maxAge option by echenley · Pull Request #241 · reduxjs/redux-devtools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

add maxAge option #241

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Apr 2, 2016
Merged

add maxAge option #241

merged 17 commits into from
Apr 2, 2016

Conversation

echenley
Copy link
Collaborator
@echenley echenley commented Feb 4, 2016
  • adds options parameter to instrument. Only maxAge property is used currently but could conceivably be used for other options.
  • removes oldest actions once maxAge is reached
  • should take care of memory issues caused by high-frequency actions

Usage:

DevTools.instrument({ maxAge: 30 });

@gaearon
Copy link
Contributor
gaearon commented Feb 4, 2016

Thank you for your work on this! I’ll take a look very soon.

@zalmoxisus
Copy link
Collaborator

@echenley, wouldn't it delete the INIT action? Won't it affect committing?

@echenley
Copy link
Collaborator Author
echenley commented Feb 4, 2016

@zalmoxisus

  1. What's the point of holding onto the init action? RESET, COMMIT, and ROLLBACK all recreate it:
actionsById = { 0: liftAction(INIT_ACTION) };
  1. I could remove line 343 to keep a reference to the last committed state even when it is removed from the list. Currently it's essentially auto-committing each time it overflows, which might not be so intuitive. Which behavior would users be more likely to expect?

@gaearon
Copy link
Contributor
gaearon commented Feb 4, 2016

I think the first action should always be INIT so effectively you want to squash all “too old” actions into a single INIT action.

@zalmoxisus
Copy link
Collaborator

Actually, I was going to implement the same approach for chrome extension some time ago, but came out with auto-commit for the same reason.

We need auto-commiting in the extension as we serialize and send the lifted state object, and don't want it to grow too much. Here we don't have this problem, the performance issue comes only when rendering the monitor.

So, I think better to implement it as a composable monitor. In this way we keep lifted state as is, but in the log monitor we get only the last actions, and we can change the maximum allowed actions prop or remove it on the fly (as HMR takes care of it).

You may take redux-devtools-filter-actions as an example or extend it as there we also delete actions and states and it is also a kind of filtering.

UPDATE: by auto-commiting I mean dispatching COMMIT action, not removing the last action from the history as it's done here.

@gaearon
Copy link
Contributor
gaearon commented Feb 4, 2016

Interesting. I agree it’s interesting to try doing this as a monitor and compare performance on projects where large amounts of actions accumulate very quickly. (http://erikras.github.io/redux-form is a good example, cc @erikras)

@gaearon
Copy link
Contributor
gaearon commented Feb 4, 2016

However, I still think a “rolling log” like this that doesn’t grow memory forever is useful and I’d love to see it as an option here. The PR just needs a little more work to squash the old actions into a single INIT action.

@zalmoxisus
Copy link
Collaborator

@gaearon, I tested this here (though for the extension), even added there redux-router as it adds extra weight with circular dependencies. Most all the CPU was taken by the app's rendering. But we need another example with just dispatching actions without UI.

@zalmoxisus
Copy link
Collaborator

Yes, it will be useful to keep it here also for persisting state. If we keep it here, wouldn't it be better to pass this limit value as a prop of the created <DevTools/> component, the same as we passing the store (when not using context)? So hot reloading could take care of it when changing the limit value on the fly? I may be mistaken and HMR will not work for this case as well.

@echenley
Copy link
Collaborator Author
echenley commented Feb 5, 2016

What about adding an optional property to the commit action, either an index, or a number of actions to commit which defaults to all? That way any monitor can commit actions explicitly. I'll fiddle with it and see how it feels.

Thinking something like:

case ActionTypes.COMMIT: {
  if (liftedAction.index) {
    // commit only up to liftedAction.index
    // keep rest of history
  } else {
    // commit everything
  }
}

@echenley
Copy link
Collaborator Author
echenley commented Feb 5, 2016

@gaearon I updated the code to always keep @@INIT at the top and just auto-commit the action after it.

@zalmoxisus It does seem like HMR doesn't work with this, though it's probably not something that would be changed much. I'd be happy to attempt a different approach in another PR.

@zalmoxisus
Copy link
Collaborator

@echenley, thanks for investigating this! After merging, I'll try to get it work with the extension to be able to change the value on the fly.

@gaearon
Copy link
Contributor
gaearon commented Feb 8, 2016
  1. Set maxAge to 5
  2. Make an error in reducer
  3. Dispatch 10 actions

Actual: the first 5 actions are skipped because autocommit happened during the error state.

Expected: I don’t think actions are ever expected to disappear. I think that auto-committing should only go as far as the last non-erroring action. If a reducer 8000 threw, that action should not be auto-committed until it is fixed. However once it is fixed, auto-commit should take action immediately if we are over maxAge.

@gaearon
Copy link
Contributor
gaearon commented Feb 8, 2016

Also, it seems like we are not taking care of currentStateIndex. It is used by monitors like sliders. We want to make sure that if it is earlier than the auto-committed action, that we move it to be at least the minimal action index that still exists.

@gaearon
Copy link
Contributor
gaearon commented Feb 8, 2016

Finally, we want to run more tests. A few tests for this specific functionality does not seem enough. We might want to add a copy for every “normal” test that uses maxAge and shows how it affects the expected behavior. If we did that, we would not have the “surprises” I mentioned above.

@echenley
Copy link
Collaborator Author

Thanks for the feedback! Making progress on this. The issue with currentStateIndex should be fixed now. I'll be adding more tests in the next couple days and cleaning up a bit, but the functionality is there.

Here's a gif of the error behavior. I have maxAge set to 3. It doesn't commit any actions once it hits an error, but will auto-commit them when the error is fixed and the reducer is reloaded. Does this look right to you?

devtools

@echenley
Copy link
Collaborator Author

I think the tests are fairly complete now. In instrument.js, I extracted the auto-commit logic into a function since it has to run when performing actions and when reloading the reducer. This will also make it pretty trivial to allow the commit action to accept either an index or # of actions to commit as previously mentioned (i.e. just call that function with the number or actions to commit).

@gaearon gaearon mentioned this pull request Feb 13, 2016
Closed
@amay
Copy link
amay commented Feb 22, 2016

How's this looking for release? I have an app with hi-frequency actions and this change is exactly what I'm looking for. I tested this branch out and I'm happy to report it fixes my problem. Now I just need a release to point to. Thanks for the good work!

@amay
Copy link
amay commented Mar 7, 2016

@echenley any update on merging this?

@echenley
Copy link
Collaborator Author
echenley commented Mar 7, 2016

@amay I haven't heard anything. I imagine @gaearon is busy with his bazillion other repos. 😉

@Mike-Sinistralis
Copy link

Just to note, this solves issues with having an onChange handler tied to the store, which causes the filter monitor to lag the application like crazy after a few seconds of typing normally. Filter solves this as well, but I would prefer both knowing that there is a performance hit after a certain number of messages. It's throwing off testing slightly.

@gaearon gaearon merged commit b7f0b26 into reduxjs:master Apr 2, 2016
@gaearon
Copy link
Contributor
gaearon commented Apr 2, 2016

That’s awesome, thank you for hard work. 👍

@gaearon
Copy link
Contributor
gaearon commented Apr 2, 2016

@echenley I added you as a collaborator. Please feel free to edit the documentation to include information about maxAge!

@echenley
Copy link
Collaborator Author
echenley commented Apr 2, 2016

Excellent, thank you!

@zalmoxisus zalmoxisus mentioned this pull request Apr 3, 2016
Closed
@calesce calesce mentioned this pull request Apr 4, 2016
Closed
@mess0615
Copy link

I don't know if this is related, but lately I keep getting a lot of maxAge output in the console such as:

maxAge 50 50

Is this leftover debugging code?

@echenley
Copy link
Collaborator Author

@mess0615 This PR didn't introduce any debug statements. Not sure where it's coming from, but it's not this repo. https://github.com/gaearon/redux-devtools/search?utf8=%E2%9C%93&q=console

@rnsloan
Copy link
rnsloan commented Apr 13, 2016

@mess0615 looks like it is coming from the Chrome extension https://chrome.google.com/webstore/detail/redux-devtools/lmhkpmbekcpmknklioeibfkpmmfibljd

@kbsymanz
Copy link

I am getting these maxAge messages too. Just started today with no apparent relationship to code that I have changed. The messages stop when I disable Redux Dev Tools in Chrome.

Found this code in the Chrome tools that seems related.

            case g.PERFORM_ACTION:
                console.log("maxAge", r.maxAge, f.length),
                r.maxAge && f.length === r.maxAge && u(1),
                _ === f.length - 1 && _++;
                var M = d++;
                p[M] = c,
                f = [].concat(f, [M]),
                E = f.length - 1;
                break;

@rnsloan
Copy link
rnsloan commented Apr 13, 2016

Chrome extension issue zalmoxisus/redux-devtools-extension#89

@zalmoxisus
Copy link
Collaborator

Oh, sorry, just forgot to remove the console log on Chrome extension. It is not related to this PR in any way.

UPD: Fixed, v1.0.0.19 is available on Chrome Store. Chrome will autoupdate it in several hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@echenley @gaearon @zalmoxisus @amay @Mike-Sinistralis @mess0615 @rnsloan @kbsymanz
0