1462390 - Extract history from JSTerm component

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

In order to clean up Console code base and make it more inline with React/Redux conventions we could extract support for history of evaluated expression from JSTerm component.

Some comments:

- There can be new history reducer initialized from asyncStorage ("devtools/shared/async-storage"). It's currently used here [1]. This new reducer can be later used for History side panel and reverse-search.

- There can be set of new Redux actions like: clearHistory, storeHistory, appendToHistory, etc.

- Harder part might be extracting the keyPress related code [2]. Individual key handlers might be also turned into Redux actions.

Honza


[1] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/devtools/client/webconsole/components/JSTerm.js#210

[2] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/devtools/client/webconsole/components/JSTerm.js#624

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1462390 - Extract history from JSTerm component;

Review commit: https://reviewboard.mozilla.org/r/247478/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247478/

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7347a78a306dbe6b12df06b14c4c3707aef4ff60

Honza

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8981337 [details]
Bug 1462390 - Extract history from JSTerm component;

https://reviewboard.mozilla.org/r/247478/#review253430

Thanks Honza, this looks a lot better.
I have some questions and comments that think need to be addressed before we land this. but nothing too big.

::: devtools/client/inspector/test/browser_inspector_menu-04-use-in-console.js:48
(Diff revision 1)
>  
>      result = await jsterm.execute();
>      isnot(result.textContent.indexOf('<p id="console-var-multi">'), -1,
>            "variable temp1 references correct node");
>  
> -    jsterm.clearHistory();
> +    jsterm.props.clearHistory();

It looks brittle to call something that is a prop on a Component.
Maybe we could have a dispatchClearHistory function in console-output-wrapper, similar to other dispatchXXX methods we already have there.

::: devtools/client/webconsole/actions/history.js:50
(Diff revision 1)
> +function loadHistory() {
> +  return (dispatch, state) => {
> +    asyncStorage.getItem("webConsoleHistory").then(value => {
> +      if (Array.isArray(value)) {
> +        dispatch({
> +          type: LOAD_HISTORY,

could this be renamed HISTORY_LOADED ? It would better convey what it does

::: devtools/client/webconsole/actions/history.js:58
(Diff revision 1)
> +/**
> + * Store history into AsyncStorage.
> + */
> +function storeHistory() {
> +  return {
> +    type: STORE_HISTORY,
> +  };
> +}

Looking at the code, I don't think this action is ever used. Could we remove it if this is the case ?

::: devtools/client/webconsole/components/JSTerm.js:41
(Diff revision 1)
> +// History Modules
> +const {
> +  getHistory,
> +  getHistoryValue
> +} = require("devtools/client/webconsole/selectors/history");
> +const HistoryActions = require("devtools/client/webconsole/actions/history");

nit: could this be `historyActions` ? I think we only capitalize first letter for class/components usually

::: devtools/client/webconsole/components/JSTerm.js:87
(Diff revision 1)
>        hud,
>      } = props;
>  
>      this.hud = hud;
>      this.hudId = this.hud.hudId;
> -    this.inputHistoryCount = Services.prefs.getIntPref(PREF_INPUT_HISTORY_COUNT);
> +    this.props.loadHistory();

I feel like we should do that when we initialize the store, in store.js, like we do for retrieving preferences for instance.
What do you think ?

::: devtools/client/webconsole/components/JSTerm.js:200
(Diff revision 1)
> -    // XXX: For now, everything is handled in an imperative way and we only want React
> -    // to do the initial rendering of the component.
> +    // XXX: For now, everything is handled in an imperative way and we
> +    // only want React to do the initial rendering of the component.
>      // This should be modified when the actual refactoring will take place.
>      return false;

This feels a bit odd since we do have some redux state now. But since we call setValue, we're getting the direct input reference.
I think this is okay for now, and we can handle it when we also handle setValue in a more React way

::: devtools/client/webconsole/components/JSTerm.js:1301
(Diff revision 1)
> +function mapDispatchToProps(dispatch) {
> +  return {
> +    appendToHistory: (expr) => dispatch(HistoryActions.appendToHistory(expr)),
> +    clearHistory: () => dispatch(HistoryActions.clearHistory()),
> +    loadHistory: () => dispatch(HistoryActions.loadHistory()),
> +    storeHistory: () => dispatch(HistoryActions.storeHistory()),

If I read the code correctly, I don't think we use this action from the jsterm

::: devtools/client/webconsole/constants.js:145
(Diff revision 1)
> +  HISTORY_BACK: -1,
> +  HISTORY_FORWARD: 1,

I don't think we make uses of those -1|1 numbers (and it's better like that). Could we replace those with Symbol() ?

::: devtools/client/webconsole/reducers/history.js:32
(Diff revision 1)
> +    // Holds the number of entries in history. This value is incremented
> +    // in this.execute().
> +    index: 0,

how is it different from entries.length ?

::: devtools/client/webconsole/reducers/history.js:37
(Diff revision 1)
> +    // Holds the number of entries in history. This value is incremented
> +    // in this.execute().
> +    index: 0,
> +
> +    // Holds the index of the history entry that the user is currently
> +    // viewing. This is reset to this.history.length when this.execute()

this.execute does not exist here

::: devtools/client/webconsole/reducers/history.js:39
(Diff revision 1)
> +    index: 0,
> +
> +    // Holds the index of the history entry that the user is currently
> +    // viewing. This is reset to this.history.length when this.execute()
> +    // is invoked.
> +    placeHolder: 0,

So, I know this is how it was done before, but I find this a bit weird. By default, the user isn't seeing an history entry at all, but "live" content. So I think having undefined or null here would better represent what the current state is

::: devtools/client/webconsole/reducers/history.js:41
(Diff revision 1)
> +    // Max number of entries in history list.
> +    historyCount: Services.prefs.getIntPref(PREF_INPUT_HISTORY_COUNT),

could this be placed and handle in the prefs reducer ? Then we can use it here, like we do in the messages reducer https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/devtools/client/webconsole/reducers/messages.js#148

::: devtools/client/webconsole/reducers/history.js:71
(Diff revision 1)
> +  // Save the current history state into async-storage when modified
> +  // and one of the listed actions is fired.
> +  if (newState != state && triggerStoreActions.includes(type)) {
> +    asyncStorage.setItem("webConsoleHistory", newState.entries);
> +  }

Could this be handled in a middleware ? Most of our side-effects actions calls (e.g. clear the messages cache) are done this way, and this keeps reducers pure.

Also, it would make the reducer code slightly lighter, as we could return directly inside the switch cases.

::: devtools/client/webconsole/reducers/history.js:81
(Diff revision 1)
> +  state = Object.assign({}, state);
> +  state.entries[state.index++] = expression;
> +  state.placeHolder = state.entries.length;

could we use the spread operator ?

```
state = {
  ...state,
  entries: [...state.entries, expression],
  placeHolder: undefined,
}
```

::: devtools/client/webconsole/reducers/history.js:82
(Diff revision 1)
> +  return newState;
> +}
> +
> +function appendToHistory(state, expression) {
> +  state = Object.assign({}, state);
> +  state.entries[state.index++] = expression;

it would be really nice if we could only push to entries without dealing with state.index

::: devtools/client/webconsole/reducers/history.js:87
(Diff revision 1)
> +  state.entries[state.index++] = expression;
> +  state.placeHolder = state.entries.length;
> +
> +  if (state.entries.length > state.historyCount) {
> +    state.entries.splice(0, state.entries.length - state.historyCount);
> +    state.index = state.placeHolder = state.entries.length;

1. it would be nice to not have to handle state.index
2. state.plateHolder could be reset to null|undefined since the user won't see any history entry anymore

::: devtools/client/webconsole/reducers/history.js:114
(Diff revision 1)
> +    state = Object.assign({}, state);
> +
> +    // Save the current input value as the latest entry in history, only if
> +    // the user is already at the last entry.
> +    // Note: this code does not store changes to items that are already in
> +    // history.
> +    if (state.placeHolder == state.index) {
> +      state.entries[state.index] = expression || "";
> +    }
> +
> +    state.placeHolder--;

maybe we can directly return from here ? We don't do anything out of the if block: 

```js
return {
  ...state,
  placeHolder: state.placeHolder - 1,
  entries: expression !== "" && (state.placeHolder == state.entries.length || state.placeHolder == undefined)
    ? [...entries, expression]
    : state.entries
}
```

::: devtools/client/webconsole/reducers/history.js:130
(Diff revision 1)
> +    state = Object.assign({}, state);
> +    state.placeHolder++;

Let's return directly from there
```js
return {
  ...state,
  placeHolder: placeHolder + 1
}
```

::: devtools/client/webconsole/selectors/history.js:31
(Diff revision 1)
> +function getNextHistoryValue(state) {
> +  if (state.placeHolder < (state.entries.length - 1)) {
> +    return state.entries[state.placeHolder + 1];
> +  }
> +  return null;
> +}
> +
> +function getPreviousHistoryValue(state) {
> +  if (state.placeHolder > 0) {
> +    return state.entries[state.placeHolder - 1];
> +  }
> +  return null;
> +}

In the other selectors, we always pass the root state, but here it seems that we assume that state is in fact the history state.
For consistency, I think we should also assume that the root state is passed

::: devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_return_key_no_selection.js:51
(Diff revision 1)
>  
>    ok(!popup.isOpen, "popup is not open after KEY_Enter");
>    is(jsterm.getInputValue(), "", "inputNode is empty after KEY_Enter");
>    is(completeNode.value, "", "completeNode is empty");
> -  is(jsterm.history[jsterm.history.length - 1], "window.testBug",
> +
> +  const entries = getHistoryEntries(jsterm.props);

This feel quite weird to me, we are calling a selector with props, whereas we tend to call them with the root state.
We can retrieve the state from console-output-wrapper, like here: https://searchfox.org/mozilla-central/source/devtools/client/webconsole/test/mochitest/browser_webconsole_network_messages_expand.js#125

We should do the same for the 2 inspector tests at the beginning of this review

::: devtools/client/webconsole/test/mochitest/browser_jsterm_history_persist.js:29
(Diff revision 1)
>  
>    // First tab: run a bunch of commands and then make sure that you can
>    // navigate through their history.
>    let hud1 = await openNewTabAndConsole(TEST_URI);
>  
> -  is(JSON.stringify(hud1.jsterm.history), "[]", "No history on first tab initially");
> +  is(JSON.stringify(getHistoryEntries(hud1.jsterm.props)),

same thing in this file, let's use the root store to access the history

::: devtools/client/webconsole/test/mochitest/head.js:73
(Diff revision 1)
>    let hud = toolbox.getCurrentPanel().hud;
>    hud.jsterm._lazyVariablesView = false;
>  
>    if (clearJstermHistory) {
>      // Clearing history that might have been set in previous tests.
> -    await hud.jsterm.clearHistory();
> +    await hud.jsterm.props.clearHistory();

It looks brittle to call something that is a prop on a Component.
Maybe we could have a dispatchClearHistory function in console-output-wrapper, similar to other dispatchXXX methods we already have there.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the review Nicolas!


(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> devtools/client/inspector/test/browser_inspector_menu-04-use-in-console.js:48
> (Diff revision 1)
> >  
> >      result = await jsterm.execute();
> >      isnot(result.textContent.indexOf('<p id="console-var-multi">'), -1,
> >            "variable temp1 references correct node");
> >  
> > -    jsterm.clearHistory();
> > +    jsterm.props.clearHistory();
> 
> It looks brittle to call something that is a prop on a Component.
> Maybe we could have a dispatchClearHistory function in
> console-output-wrapper, similar to other dispatchXXX methods we already have
> there.
Done, `dispatchClearHistory` method introduced

> ::: devtools/client/webconsole/actions/history.js:50
> (Diff revision 1)
> > +function loadHistory() {
> > +  return (dispatch, state) => {
> > +    asyncStorage.getItem("webConsoleHistory").then(value => {
> > +      if (Array.isArray(value)) {
> > +        dispatch({
> > +          type: LOAD_HISTORY,
> 
> could this be renamed HISTORY_LOADED ? It would better convey what it does
Done

> ::: devtools/client/webconsole/actions/history.js:58
> (Diff revision 1)
> > +/**
> > + * Store history into AsyncStorage.
> > + */
> > +function storeHistory() {
> > +  return {
> > +    type: STORE_HISTORY,
> > +  };
> > +}
> 
> Looking at the code, I don't think this action is ever used. Could we remove
> it if this is the case ?
True, removed

> ::: devtools/client/webconsole/components/JSTerm.js:41
> (Diff revision 1)
> > +// History Modules
> > +const {
> > +  getHistory,
> > +  getHistoryValue
> > +} = require("devtools/client/webconsole/selectors/history");
> > +const HistoryActions = require("devtools/client/webconsole/actions/history");
> 
> nit: could this be `historyActions` ? I think we only capitalize first
> letter for class/components usually
Done

> ::: devtools/client/webconsole/components/JSTerm.js:87
> (Diff revision 1)
> >        hud,
> >      } = props;
> >  
> >      this.hud = hud;
> >      this.hudId = this.hud.hudId;
> > -    this.inputHistoryCount = Services.prefs.getIntPref(PREF_INPUT_HISTORY_COUNT);
> > +    this.props.loadHistory();
> 
> I feel like we should do that when we initialize the store, in store.js,
> like we do for retrieving preferences for instance.
> What do you think ?
Done.

The entire load/save history logic and access to asyncStorage is now
wrapped in new Redux middleware function (`historyPersistenceMiddleware`).

> ::: devtools/client/webconsole/components/JSTerm.js:200
> (Diff revision 1)
> > -    // XXX: For now, everything is handled in an imperative way and we only want React
> > -    // to do the initial rendering of the component.
> > +    // XXX: For now, everything is handled in an imperative way and we
> > +    // only want React to do the initial rendering of the component.
> >      // This should be modified when the actual refactoring will take place.
> >      return false;
> 
> This feels a bit odd since we do have some redux state now. But since we
> call setValue, we're getting the direct input reference.
> I think this is okay for now, and we can handle it when we also handle
> setValue in a more React way
Exactly, fixing this should again simplify the entire JSTerm component.


> ::: devtools/client/webconsole/components/JSTerm.js:1301
> (Diff revision 1)
> > +function mapDispatchToProps(dispatch) {
> > +  return {
> > +    appendToHistory: (expr) => dispatch(HistoryActions.appendToHistory(expr)),
> > +    clearHistory: () => dispatch(HistoryActions.clearHistory()),
> > +    loadHistory: () => dispatch(HistoryActions.loadHistory()),
> > +    storeHistory: () => dispatch(HistoryActions.storeHistory()),
> 
> If I read the code correctly, I don't think we use this action from the
> jsterm
Removed

> ::: devtools/client/webconsole/constants.js:145
> (Diff revision 1)
> > +  HISTORY_BACK: -1,
> > +  HISTORY_FORWARD: 1,
> 
> I don't think we make uses of those -1|1 numbers (and it's better like
> that). Could we replace those with Symbol() ?
These constants are used (redefined) in browser_jsterm_history.js test

We could require the 'constants' module in this test, but we'd have to use the same window loader. Similarly how the Network panel is doing it when requiring modules in tests. See e.g. openMessageInNetmonitor() in head.js 

```js
let { store, windowRequire } = panelWin;
let nmActions = windowRequire("devtools/client/netmonitor/src/actions/index");
```

I can file a follow up for that.


> ::: devtools/client/webconsole/reducers/history.js:32
> (Diff revision 1)
> > +    // Holds the number of entries in history. This value is incremented
> > +    // in this.execute().
> > +    index: 0,
> 
> how is it different from entries.length ?
Yeah, this is confusing, it's used for the case where the user goes back in the history and an empty string is added at then end, so it's possible to go forward again and reach an empty line. In case where there is an initial string on the command line it should be reachable too.

I filled bug 1465352 to cover this.

> ::: devtools/client/webconsole/reducers/history.js:37
> (Diff revision 1)
> > +    // Holds the number of entries in history. This value is incremented
> > +    // in this.execute().
> > +    index: 0,
> > +
> > +    // Holds the index of the history entry that the user is currently
> > +    // viewing. This is reset to this.history.length when this.execute()
> 
> this.execute does not exist here
Comment updated

> ::: devtools/client/webconsole/reducers/history.js:39
> (Diff revision 1)
> > +    index: 0,
> > +
> > +    // Holds the index of the history entry that the user is currently
> > +    // viewing. This is reset to this.history.length when this.execute()
> > +    // is invoked.
> > +    placeHolder: 0,
> 
> So, I know this is how it was done before, but I find this a bit weird. By
> default, the user isn't seeing an history entry at all, but "live" content.
> So I think having undefined or null here would better represent what the
> current state is
Done, set to undefined.

> ::: devtools/client/webconsole/reducers/history.js:41
> (Diff revision 1)
> > +    // Max number of entries in history list.
> > +    historyCount: Services.prefs.getIntPref(PREF_INPUT_HISTORY_COUNT),
> 
> could this be placed and handle in the prefs reducer ? Then we can use it
> here, like we do in the messages reducer
> https://searchfox.org/mozilla-central/rev/
> 5a744713370ec47969595e369fd5125f123e6d24/devtools/client/webconsole/reducers/
> messages.js#148
Good point, done!

> ::: devtools/client/webconsole/reducers/history.js:71
> (Diff revision 1)
> > +  // Save the current history state into async-storage when modified
> > +  // and one of the listed actions is fired.
> > +  if (newState != state && triggerStoreActions.includes(type)) {
> > +    asyncStorage.setItem("webConsoleHistory", newState.entries);
> > +  }
> 
> Could this be handled in a middleware ? Most of our side-effects actions
> calls (e.g. clear the messages cache) are done this way, and this keeps
> reducers pure.
Yes, done.

Btw. what about splitting the store.js file and put individual enhancer
and middleware functions into separate modules/files? They could be stored in new
middleware and enhancers directories. Would be small modules, but
some of them could crystallize into shared modules later.

> ::: devtools/client/webconsole/reducers/history.js:81
> (Diff revision 1)
> > +  state = Object.assign({}, state);
> > +  state.entries[state.index++] = expression;
> > +  state.placeHolder = state.entries.length;
> 
> could we use the spread operator ?
> 
> ```
> state = {
>   ...state,
>   entries: [...state.entries, expression],
>   placeHolder: undefined,
> }
> ```
Done as follows:

  let newEntries = [...entries, ...state.entries];
  return {
    ...state,
    entries: newEntries,
    placeHolder: newEntries.length,
  };

> ::: devtools/client/webconsole/reducers/history.js:87
> (Diff revision 1)
> > +  state.entries[state.index++] = expression;
> > +  state.placeHolder = state.entries.length;
> > +
> > +  if (state.entries.length > state.historyCount) {
> > +    state.entries.splice(0, state.entries.length - state.historyCount);
> > +    state.index = state.placeHolder = state.entries.length;
> 
> 1. it would be nice to not have to handle state.index
> 2. state.plateHolder could be reset to null|undefined since the user won't
> see any history entry anymore
This can be done in a follow up as mentioned above.

> ::: devtools/client/webconsole/reducers/history.js:114
> (Diff revision 1)
> > +    state = Object.assign({}, state);
> > +
> > +    // Save the current input value as the latest entry in history, only if
> > +    // the user is already at the last entry.
> > +    // Note: this code does not store changes to items that are already in
> > +    // history.
> > +    if (state.placeHolder == state.index) {
> > +      state.entries[state.index] = expression || "";
> > +    }
> > +
> > +    state.placeHolder--;
> 
> maybe we can directly return from here ? We don't do anything out of the if
> block: 
> 
> ```js
> return {
>   ...state,
>   placeHolder: state.placeHolder - 1,
>   entries: expression !== "" && (state.placeHolder == state.entries.length
> || state.placeHolder == undefined)
>     ? [...entries, expression]
>     : state.entries
> }
> ```
I did some changes to the code (not exactly what you suggested),
but it looks better now.

> ::: devtools/client/webconsole/selectors/history.js:31
> (Diff revision 1)
> > +function getNextHistoryValue(state) {
> > +  if (state.placeHolder < (state.entries.length - 1)) {
> > +    return state.entries[state.placeHolder + 1];
> > +  }
> > +  return null;
> > +}
> > +
> > +function getPreviousHistoryValue(state) {
> > +  if (state.placeHolder > 0) {
> > +    return state.entries[state.placeHolder - 1];
> > +  }
> > +  return null;
> > +}
> 
> In the other selectors, we always pass the root state, but here it seems
> that we assume that state is in fact the history state.
> For consistency, I think we should also assume that the root state is passed
Done

> devtools/client/webconsole/test/mochitest/
> browser_jsterm_autocomplete_return_key_no_selection.js:51
> (Diff revision 1)
> >  
> >    ok(!popup.isOpen, "popup is not open after KEY_Enter");
> >    is(jsterm.getInputValue(), "", "inputNode is empty after KEY_Enter");
> >    is(completeNode.value, "", "completeNode is empty");
> > -  is(jsterm.history[jsterm.history.length - 1], "window.testBug",
> > +
> > +  const entries = getHistoryEntries(jsterm.props);
> 
> This feel quite weird to me, we are calling a selector with props, whereas
> we tend to call them with the root state.
> We can retrieve the state from console-output-wrapper, like here:
> https://searchfox.org/mozilla-central/source/devtools/client/webconsole/test/
> mochitest/browser_webconsole_network_messages_expand.js#125
> 
> We should do the same for the 2 inspector tests at the beginning of this
> review
Done

> :::
> devtools/client/webconsole/test/mochitest/browser_jsterm_history_persist.js:
> 29
> (Diff revision 1)
> >  
> >    // First tab: run a bunch of commands and then make sure that you can
> >    // navigate through their history.
> >    let hud1 = await openNewTabAndConsole(TEST_URI);
> >  
> > -  is(JSON.stringify(hud1.jsterm.history), "[]", "No history on first tab initially");
> > +  is(JSON.stringify(getHistoryEntries(hud1.jsterm.props)),
> 
> same thing in this file, let's use the root store to access the history
Done

> ::: devtools/client/webconsole/test/mochitest/head.js:73
> (Diff revision 1)
> >    let hud = toolbox.getCurrentPanel().hud;
> >    hud.jsterm._lazyVariablesView = false;
> >  
> >    if (clearJstermHistory) {
> >      // Clearing history that might have been set in previous tests.
> > -    await hud.jsterm.clearHistory();
> > +    await hud.jsterm.props.clearHistory();
> 
> It looks brittle to call something that is a prop on a Component.
> Maybe we could have a dispatchClearHistory function in
> console-output-wrapper, similar to other dispatchXXX methods we already have
> there.
Done

Honza

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8981337 [details]
Bug 1462390 - Extract history from JSTerm component;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247478/diff/1-2/

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8981337 [details]
Bug 1462390 - Extract history from JSTerm component;

https://reviewboard.mozilla.org/r/247478/#review253964

Thanks Honza, that looks good ! I only have a few nit, so feel free to land with a green try

::: devtools/client/webconsole/components/JSTerm.js:805
(Diff revision 2)
>    historyPeruse(direction) {
> -    if (!this.history.length) {
> -      return false;
> -    }
> +    let {
> +      history,
> +      updatePlaceHolder,
> +      historyPeruse,

nit: could we find another name either for the JsTerm method or for the props ? I find it confusing navigating through code where 2 function with the same name in fact represent different things

::: devtools/client/webconsole/reducers/history.js:74
(Diff revision 2)
> +function clearHistory(state) {
> +  return getInitialState();
> +}
> +
> +function historyLoaded(state, entries) {
> +  let newEntries = [...entries, ...state.entries];

nit: add a comment saying that we push the history entries before the ones that were added to the state in this session

::: devtools/client/webconsole/reducers/prefs.js:11
(Diff revision 2)
>  "use strict";
>  
>  const PrefState = (overrides) => Object.freeze(Object.assign({
>    logLimit: 1000,
> -  sidebarToggle: false
> +  sidebarToggle: false,
> +  historyCount: 50,

you'll probably have a confluct when Bug 983473 land

::: devtools/client/webconsole/selectors/history.js:25
(Diff revision 2)
> +}
> +
> +function getHistoryValue(state, direction) {
> +  if (direction == HISTORY_BACK) {
> +    return getPreviousHistoryValue(state);
> +  } else if (direction == HISTORY_FORWARD) {

could we simply have an `if` (since we return in the first if) ? 
I thought we had an eslint rule for that

::: devtools/client/webconsole/store.js:113
(Diff revision 2)
>    };
>  }
>  
>  function createRootReducer() {
>    return function rootReducer(state, action) {
>      // We want to compute the new state for all properties except "messages".

and "history" now

::: devtools/client/webconsole/store.js:344
(Diff revision 2)
> +function historyPersistenceMiddleware(store) {
> +  asyncStorage.getItem("webConsoleHistory").then(value => {
> +    if (Array.isArray(value)) {
> +      store.dispatch(historyActions.historyLoaded(value));
> +    }
> +  }, console.error);
> +
> +  return next => action => {
> +    const res = next(action);
> +
> +    let triggerStoreActions = [
> +      APPEND_TO_HISTORY,
> +      CLEAR_HISTORY,
> +    ];
> +
> +    // Save the current history entries when modified.
> +    if (triggerStoreActions.includes(action.type)) {
> +      const state = store.getState();
> +      asyncStorage.setItem("webConsoleHistory", state.history.entries);
> +    }
> +
> +    return res;
> +  };
> +}

that's great :)

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

> Yeah, this is confusing, it's used for the case where the user goes back in the history and an empty string is added at then end, so it's possible to go forward again and reach an empty line. In case where there is an initial string on the command line it should be reachable too.
> I filled bug 1465352 to cover this.

Great

> Btw. what about splitting the store.js file and put individual enhancer and middleware functions into separate modules/files?  they could be stored in new middleware and enhancers directories. Would be small modules, but some of them could crystallize into shared modules later.

Yes, that would be nice I think, as store.js is becoming a bit big.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8981337 [details]
Bug 1462390 - Extract history from JSTerm component;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247478/diff/2-3/

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #6)
> ::: devtools/client/webconsole/components/JSTerm.js:805
> (Diff revision 2)
> >    historyPeruse(direction) {
> > -    if (!this.history.length) {
> > -      return false;
> > -    }
> > +    let {
> > +      history,
> > +      updatePlaceHolder,
> > +      historyPeruse,
> 
> nit: could we find another name either for the JsTerm method or for the
> props ? I find it confusing navigating through code where 2 function with
> the same name in fact represent different things
Yep, renamed to: 'getValueFromHistory'

> ::: devtools/client/webconsole/reducers/history.js:74
> (Diff revision 2)
> > +function clearHistory(state) {
> > +  return getInitialState();
> > +}
> > +
> > +function historyLoaded(state, entries) {
> > +  let newEntries = [...entries, ...state.entries];
> 
> nit: add a comment saying that we push the history entries before the ones
> that were added to the state in this session
Done

> ::: devtools/client/webconsole/selectors/history.js:25
> (Diff revision 2)
> > +}
> > +
> > +function getHistoryValue(state, direction) {
> > +  if (direction == HISTORY_BACK) {
> > +    return getPreviousHistoryValue(state);
> > +  } else if (direction == HISTORY_FORWARD) {
> 
> could we simply have an `if` (since we return in the first if) ? 
> I thought we had an eslint rule for that
Done

> ::: devtools/client/webconsole/store.js:113
> (Diff revision 2)
> >    };
> >  }
> >  
> >  function createRootReducer() {
> >    return function rootReducer(state, action) {
> >      // We want to compute the new state for all properties except "messages".
> 
> and "history" now
Fixed

Thanks Nicolas!

Honza

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)
> > Btw. what about splitting the store.js file and put individual enhancer and middleware functions into separate modules/files?  they could be stored in new middleware and enhancers directories. Would be small modules, but some of them could crystallize into shared modules later.
> 
> Yes, that would be nice I think, as store.js is becoming a bit big.
Created bug 1465455

Honza

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8981337 [details]
Bug 1462390 - Extract history from JSTerm component;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247478/diff/3-4/

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56e1a64c81a63a8a00fa291069e4b0b072a40c17&selectedJob=181065772

Honza

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8981337 [details]
Bug 1462390 - Extract history from JSTerm component;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247478/diff/4-5/

Comment 14

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 6463b73324461ba565e60e1848e9d062e3826a62 -d 437a8d7c128a: rebasing 466241:6463b7332446 "Bug 1462390 - Extract history from JSTerm component; r=nchevobbe" (tip)
merging devtools/client/webconsole/components/JSTerm.js
merging devtools/client/webconsole/constants.js
merging devtools/client/webconsole/reducers/prefs.js
merging devtools/client/webconsole/store.js
merging devtools/client/webconsole/webconsole-output-wrapper.js
warning: conflicts while merging devtools/client/webconsole/components/JSTerm.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/client/webconsole/constants.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/client/webconsole/reducers/prefs.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/client/webconsole/store.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8981337 [details]
Bug 1462390 - Extract history from JSTerm component;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247478/diff/5-6/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f3835d852c0
Extract history from JSTerm component; r=nchevobbe

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2f3835d852c0