chore(sdk): refactor Filter into different modules
#1968
Conversation
size-limit report 📦
|
| @@ -0,0 +1,57 @@ | |||
| import { FilterCore } from "@waku/core"; | |||
There was a problem hiding this comment.
I don't think we gain anything by creating SubscriptionManager entity - considering FilterSDK is essentially a manager for subscriptions from FilterCore.
Overall our implementations are simple enough not to make any new classes.
For now goal is to have Core and SDK decoupled - let's formalize other improvements in separate task stream.
There was a problem hiding this comment.
IMO the current Filter implementation is very tightly coupled and this is a change towards bringing maintainability to the codebase. I believe the Filter implementation is complex enough to be seen as simple currently.
I also anticipate introduction of more logic to SubscriptionManager like resubscription for reliability & abstracting each Subscription object with `PubsubTopic
Do you see any cons to this approach?
There was a problem hiding this comment.
Adding on:
FilterSDKis meant to be the interface to interact with theFilterprotocol -> new APIs etc would exist hereSubscriptionManageris responsible for allSubscriptionobject handling related logicSubscriptionis responsible for handling aSubscriptiongranularly likeping,unsubscribeetc
IMO this only increases readability and maintainability
Do you disagree? What alternative would you suggest?
There was a problem hiding this comment.
I understand that decoupling can be beneficial to readability etc.
The thing I am pointing out is - this PR tries to optimize entity that is 150 lines long, which is not that concerning to any of the points mentioned.
I would suggest not to do these improvements unless they are impactful. For example:
- implementation is big - more than
500lines - we can addeslintto control it (but we don't have this kind of files for now); - hard to maintain - when covering with unit test or making changes - which also brings us to
big fileproblem;
Overall - I think when improvements are done before bad patterns in the code appear - they only make things harder to maintain. Basic case is - in worst case now someone would need to touch 3 files instead of one.
As action point - we can intro eslint rule to prevent big files from appearing.
There was a problem hiding this comment.
Thanks for your reply!
The thing I am pointing out is - this PR tries to optimize entity that is 150 lines long, which is not that concerning to any of the points mentioned.
The Filter class was ~350+ lines long -- so splitting into Subscription and Filter seems in the right direction. Would you agree? (Thus bringing FilterSDK and Subscription to be ~150 lines)
I guess the introduction of SubscriptionManager specifically is in question -- I agree to your point that 3 files would need to be touched, but personally prefer keeping each class for a Single Responsibility
I'm happy to address the PR to remove SubscriptionManager and maintain subscription handling within the FilterSDK class for now and can revisit this later.
Let me know if that's different from what you propose.
There was a problem hiding this comment.
so splitting into Subscription and Filter seems in the right direction
yes, I agree it was a good choice
I'm happy to address the PR to remove SubscriptionManager and maintain subscription handling within the FilterSDK class for now and can revisit this later.
let's do this, let's keep Subscription and Filter classes for now and revisit later
Additionally - we can add eslint to prevent big files from appearing so that it would make us refactor when it is time.
There was a problem hiding this comment.
Sounds good, thanks!
danisharora099
force-pushed
the
chore/refactor/filter
branch
from
1c2249f
to
b97cfcf
Compare
Problem
The current Filter implementation is very tightly coupled together which decreases readability, tough to refactor, and introduce new changes/APIs.
Solution
With work on #1886, we decoupled Filter into
coreandsdkThis PR splits
Filterinto: