Closed Bug 1417446 Opened 2 years ago Closed 2 years ago

Remove the MESSAGE_ADD action

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

(Whiteboard: [newconsole-mvp])

Attachments

(1 file, 2 obsolete files)

In Bug 1371721, we created a new action, MESSAGES_ADD [1] (note the plural form) to batch our messages addition into the store.
This means that we are no longer using the MESSAGE_ADD [2] (note the single form) action in our code, except in tests [3].

We should get rid of this action and only use messagesAdd in all the tests, which represent better what the console is doing.

Do be more specific, the following :

> messageAdd(packet)

should be tranformed to :

> messagesAdd([packet])

In some places we were dispatching multiple messageAdd sequentially. These could be merged into one messagesAdd.

When this patch is done, all the mocha tests need to be successful.
The tests can be run with the following command: 

> cd devtools/client/webconsole && npm test


The constant [4], as well as the reducer handling this action [5] should be removed as well.


[1] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/devtools/client/webconsole/new-console-output/actions/messages.js#29-52
[2] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/devtools/client/webconsole/new-console-output/actions/messages.js#54-71
[3] https://searchfox.org/mozilla-central/search?q=messageAdd(&case=true&regexp=false&path=
[4] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/devtools/client/webconsole/new-console-output/constants.js#16
[5] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/devtools/client/webconsole/new-console-output/reducers/messages.js#183-185
Mentor: nchevobbe
Depends on: 1417439
Keywords: good-first-bug
Priority: -- → P3
Can I work on this bug and try fixing it?
Flags: needinfo?(nchevobbe)
Hello Pradeep, 

Of course you can start working on this :)
If you have any questions, don't hesitate to hit me here or on Slack https://devtools-html-slack.herokuapp.com/
Flags: needinfo?(nchevobbe)
Assignee: nobody → pradeepgangwar39
Pradeep, did you start working on this ?
I'm afraid I did remove the action in Bug 1368100.
But if you have already a patch ready for review, I'll review it and do the rebase on my patch.
Flags: needinfo?(pradeepgangwar39)
Attached patch Bug-1417446.patch (obsolete) — Splinter Review
I tried solving it and after running npm test, I get some error all of which are associated with length property. It says cannot read property length of undefined. I was not getting any idea of error so i decided to make a WIP patch and ask for feedback.
Flags: needinfo?(pradeepgangwar39)
Attachment #8929596 - Flags: feedback?(nchevobbe)
Attached patch Bug-1417446.patch (obsolete) — Splinter Review
Attachment #8929596 - Attachment is obsolete: true
Attachment #8929596 - Flags: feedback?(nchevobbe)
Attachment #8929611 - Flags: feedback?(nchevobbe)
Comment on attachment 8929611 [details] [diff] [review]
Bug-1417446.patch

Review of attachment 8929611 [details] [diff] [review]:
-----------------------------------------------------------------

There are some errors in the patch that might explain the test failures that you see.
Also, I am sorry to say that all of this is mostly covered in a working patch for Bug 1368100. Do you mind if we close this bug ?

::: devtools/client/webconsole/new-console-output/actions/messages.js
@@ +51,4 @@
>    };
>  }
>  
> +function messagesAdd(packet, idGenerator = null) {

This function should be removed. messagesAdd already exists

@@ +142,4 @@
>  }
>  
>  module.exports = {
> +  messagesAdd,

we shouldn't export the same function twice

::: devtools/client/webconsole/new-console-output/reducers/messages.js
@@ +180,4 @@
>  
>        return limitTopLevelMessageCount(newState, logLimit);
>  
> +    case constants.MESSAGES_ADD:

this whole block should be removed, we already have a `case` for MESSAGES_ADD

::: devtools/client/webconsole/new-console-output/test/helpers.js
@@ +27,4 @@
>  
>    const idGenerator = new IdGenerator();
>    wrappedActions.messageAdd = (packet) => {
> +    return actions.messagesAdd(packet, idGenerator);

messagesAdd takes an array : actions.messagesAdd([packet])

@@ +41,4 @@
>  
>    // Add the messages from the input commands to the store.
>    input.forEach((cmd) => {
> +    store.dispatch(actions.messagesAdd(stubPackets.get(cmd)));

same here, messagesAdd takes an array
Attachment #8929611 - Flags: feedback?(nchevobbe) → feedback-
There is no problem closing this bug if this has been solved in Bug 1368100. :-)
Thank you for your understanding :)
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1368100
Assignee: pradeepgangwar39 → nchevobbe
Mentor: nchevobbe
Status: RESOLVED → REOPENED
Keywords: good-first-bug
Resolution: DUPLICATE → ---
Attachment #8929611 - Attachment is obsolete: true
Blocks: 1425577
Status: REOPENED → ASSIGNED
Priority: P3 → P1
Whiteboard: [newconsole-mvp]
Comment on attachment 8943519 [details]
Bug 1417446 - Remove MESSAGE_ADD action;.

https://reviewboard.mozilla.org/r/213860/#review219738

Thanks!
Attachment #8943519 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/fccc4ee24bd6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Were you expecting a perf improvement here?
We got a 9% speedup on the mutations test while this bug and bug 1431334 landed:
http://firefox-dev.tools/performance-dashboard/perfherder/?test=inspector.mutations&days=14&filterstddev=true
Sorry, I thought I was on the console DAMP page. But this is an improvement related to inspector.
Nevermind my previous comment!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.