-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(store)!: move protocol implementation opinions to @waku/sdk
#1913
Conversation
6a685ed
to
e98c35d
Compare
size-limit report 📦
|
] | ||
], | ||
"dependencies": { | ||
"@waku/proto": "^0.0.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should add it as dependency
to @waku/interfaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is that? 🤔
as i see it, it does not bring any breaking changes, in fact reduces redundant type declarations that can be imported from proto
such as done for Cursor
in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@waku/interface
name implies to be interfaces
only and to have little to no runtime footprint. as I mentioned somewhere in your PRs - right now we abuse enums
and use them as const
(note: this is not necessarily bad thing with enums
)
as for @waku/proto
- it has enormous runtime footprint and has nothing to do with ts
types directly
we should think of work around here:
- defining type in
@waku/interfaces
and using in@waku/proto
instead - duplicating type in
@waku/interfaces
and using it from here instead of@waku/proto
maybe you think of something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defining type in @waku/interfaces and using in @waku/proto instead
this seems challenging because proto
ts files are generated from .proto
definitions, and we shouldn't be manually updating those
duplicating type in @waku/interfaces and using it from here instead of @waku/proto
this is what we have been doing so far
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what we have been doing so far
I probably missed it. Could you, please, point me where?
I checked build artifact and it doesn't contain any footprint of proto
, that means it is smart enough to understand there is only type import
as a precaution I propose to make it devDependency
(whole @waku/interfaces
is kinda a dev dependnecy anyway) and change import { proto_store as proto } from "@waku/proto";
to something like import type { proto_store as proto } from "@waku/proto";
in store.ts
This way we ensure it is not increasing size and it doesn't leak anything else apart from types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably missed it. Could you, please, point me where?
For example: Cursor
:
@waku/proto
definition:
js-waku/packages/proto/src/lib/store.ts
Lines 11 to 16 in 0a8382e
export interface Index { | |
digest: Uint8Array | |
receiverTime: bigint | |
senderTime: bigint | |
pubsubTopic: string | |
} |
@waku/interfaces
redefinition: js-waku/packages/interfaces/src/store.ts
Lines 14 to 19 in 0a8382e
export interface Cursor { | |
digest: Uint8Array; | |
receiverTime: bigint; | |
senderTime: bigint; | |
pubsubTopic: string; | |
} |
as a precaution I propose to make it devDependency (whole @waku/interfaces is kinda a dev dependnecy anyway) and change import { proto_store as proto } from "@waku/proto"; to something like import type { proto_store as proto } from "@waku/proto"; in store.ts
This way we ensure it is not increasing size and it doesn't leak anything else apart from types
I agree with this, approach wise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's discuss
087afaf
to
d9232c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for addressing comments!
Problem
#1886
Solution
Moves away the abstraction of using multiple peers, along with the offered APIs to make queries for store by:
StoreSDK
(and renaming protocol implementation toStoreCore
StoreCore
contains one API that returns anAsyncGenerator
:*queryPerPage
StoreSDK
offers different APIs to query, that usesqueryPerPage
Notes
@waku/core
should be as unopinionated as possible #1886Contribution checklist:
!
in title if breaks public API;