[herald] Rename chronik src file to websocket.js
ClosedPublic
Actions

Authored by bytesofman on Apr 7 2023, 16:18.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCacb120d32f25: [herald] Rename chronik src file to websocket.js

Summary

T3060

Stack to refactor ecash-herald so that all functions can be unit tested

Here, renaming chronik.js to websocket.js, which better reflects what is going on in this file.

Test Plan

Confirm no other imports are using chronik.js

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bytesofman created this revision.Apr 7 2023, 16:18

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 7 2023, 16:18

bytesofman requested review of this revision.Apr 7 2023, 16:18

bytesofman added a child revision: D13615: [herald] Rename unit test file.

Harbormaster completed remote builds in B23076: Diff 39401.Apr 7 2023, 16:26

That's not a better name, you're not implementing websocket

This revision now requires changes to proceed.Apr 9 2023, 06:26

The only functions in websocket.js are initializeWebsocket and parseWebsocketMessage. Follows the same logic as alias-server

there's a case that both these files should be, say websocketMethods.js. that should be one diff that does this for both apps, outside of this stack.

My main concern here is the impact reverting this name change will have on other diffs in the stack. Granted, I should not have done it this way, but here we are.

I'm happy to open a task or change this later in the stack.

rebase

Harbormaster completed remote builds in B23155: Diff 39490.Apr 10 2023, 16:30

if the stack order is the issue then let's rename later. But the previous name was better: what do these function do that require the use of the websocket messaging protocol ?

This revision now requires changes to proceed.Apr 10 2023, 18:52

In D13614#303716, @Fabien wrote:

if the stack order is the issue then let's rename later. But the previous name was better: what do these function do that require the use of the websocket messaging protocol ?

See T3104 for tracking this issue. Since the same issue exists in alias-server, imo it should be patched in one diff applying to both apps.

bytesofman requested review of this revision.Apr 11 2023, 14:07

OK, I don't think this diff needs to be reviewed anymore

This revision now requires changes to proceed.Apr 12 2023, 08:57

In D13614#304312, @Fabien wrote:

OK, I don't think this diff needs to be reviewed anymore

changing this filename will require a potentially failing rebase in the next 16 diffs on this stack. Is it not possible to green this diff -- which follows the same convention already in alias-server -- knowing that there is a change planned that will resolve the issue?

edit: the reason I don't want to add this filename change to the end of the stack is bc it should also change alias-server, which also has an open stack.

bytesofman requested review of this revision.Apr 12 2023, 11:36

In D13614#304354, @bytesofman wrote:

In D13614#304312, @Fabien wrote:

OK, I don't think this diff needs to be reviewed anymore

changing this filename will require a potentially failing rebase in the next 16 diffs on this stack. Is it not possible to green this diff -- which follows the same convention already in alias-server -- knowing that there is a change planned that will resolve the issue?

edit: the reason I don't want to add this filename change to the end of the stack is bc it should also change alias-server, which also has an open stack.

You should really avoid building large stacks and submit them all at the same time. When some diff is ready, just submit it asap and get it landed asap to avoid this rebase hell.
Because this doesn't introduce any bug, untested code or anything that could cause breakage I will pass for once this time.

This revision is now accepted and ready to land.Apr 12 2023, 13:58

Closed by commit rABCacb120d32f25: [herald] Rename chronik src file to websocket.js (authored by bytesofman). · Explain WhyApr 12 2023, 14:26

This revision was automatically updated to reflect the committed changes.

bytesofman added a commit: rABCacb120d32f25: [herald] Rename chronik src file to websocket.js.