[chronik-client] Move appendWsUrls out of the class as a public function
ClosedPublic
Actions

Authored by alitayin on Tue, Apr 15, 09:07.

Details

Reviewers

Fabien
bytesofman
emack

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rABCcfd0d9a04c28: [chronik-client] Move appendWsUrls out of the class as a public function

Summary

Move appendWsUrls out of FailoverProxy class and export as an independent function.

Rationale:

Code simplification - the function does not depend on any internal state of FailoverProxy
Make the function reusable outside FailoverProxy scope, especially in the new node selection (ping) feature

Regarding function location:

Although the function is separated from the class, keeping it in failoverProxy.ts is reasonable because:
The function was originally designed to handle proxy URLs and is closely tied to the Endpoint interface, which is still defined in failoverProxy.ts

Note: The function is only used in tests, not found in other directories, so only need to adjust the import in test.ts.

Test Plan

npm test

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

alitayin created this revision.Tue, Apr 15, 09:07

Owners added a reviewer: Restricted Owners Package.Tue, Apr 15, 09:07

Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Apr 15, 09:07

alitayin requested review of this revision.Tue, Apr 15, 09:07

Harbormaster completed remote builds in B32966: Diff 53483.Tue, Apr 15, 09:17

alitayin retitled this revision from [chronik.e.cash] Move appendWsUrls out of the class as a public function to [chronik-client] Move appendWsUrls out of the class as a public function.Tue, Apr 15, 09:22

Please update the summary. The title is correct as it describes what is being done, but the summary does the same and should instead explain the rationale for this change. With no context the reviewer cannot decide if this is a good change or not. In this case you have 2 benefits:

It simplifies the code because this function never used the FailoverProxy, and it shouldn't have been a member from the beginning.
It makes it possible to reuse the function outside of the scope of the FailoverProxy, namely for the ping feature.

And now this raises the question: if it doesn't belong to FailoverProxy and will be used outside of the FailoverProxy, then why did you leave the function is the FailoverProxy source file ? Isn't it better suited elsewhere ? This is also something you can explain in the summary to give the reviewer confidence that you thought about this.

This revision now requires changes to proceed.Tue, Apr 15, 09:52

alitayin requested review of this revision.Tue, Apr 15, 10:08

alitayin edited the summary of this revision. (Show Details)

Fabien accepted this revision.Tue, Apr 15, 10:32

This revision is now accepted and ready to land.Tue, Apr 15, 10:32

Closed by commit rABCcfd0d9a04c28: [chronik-client] Move appendWsUrls out of the class as a public function (authored by alitayin). · Explain WhyTue, Apr 15, 10:35

This revision was automatically updated to reflect the committed changes.

alitayin added a commit: rABCcfd0d9a04c28: [chronik-client] Move appendWsUrls out of the class as a public function.

alitayin mentioned this in D17913: [chronik-client] Added a mechanism for automatically selecting the fastest responding node, supporting two strategies: AsOrdered and ClosestFirst..Tue, Apr 15, 13:20

Fabien added a child revision: D17913: [chronik-client] Added a mechanism for automatically selecting the fastest responding node, supporting two strategies: AsOrdered and ClosestFirst..Wed, Apr 16, 10:41