-
Notifications
You must be signed in to change notification settings - Fork 72
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
refactor(daemon): Consolidate interface definitions and cleanup #2225
Conversation
bb4eb80
to
5945434
Compare
b589c8c
to
eb91183
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.
LGTM! A couple of inline comments. The interfaces
refactor is most appreciated.
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.
Hm, why not interfaces/index.js
and interfaces/web.js
? Indeed, why a separate file at all?
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 loathe the Node.js index.js
lookup because it assumes a search path that doesn’t translate to the web. Of course, that’s immaterial since we’re never going to be free of a bundler or import-map, but I prefer to live in the make-believe so as to be ready for change.
I separated interfaces/web.js
because these interfaces are not canon to the daemon itself but a contract between the web server and client, and the web client should shake out the interfaces it doesn’t need in its bundle.
type: 'known-peers-store', | ||
}); | ||
// We generate formulas for some entities that are presumed to exist | ||
// because they are parts of the daemon's root object. |
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.
// because they are parts of the daemon's root object. | |
// because they are part of the daemon's root object. |
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 think the cardinality of “formulas”, “entities”, “are”, and “parts” should agree here.
5945434
to
34d45cd
Compare
eb91183
to
87fb53d
Compare
34d45cd
to
7a544bf
Compare
87fb53d
to
04ab5a9
Compare
7a544bf
to
d477162
Compare
04ab5a9
to
a800bb4
Compare
d477162
to
2557dbd
Compare
a800bb4
to
19b7de5
Compare
2557dbd
to
2347a23
Compare
19b7de5
to
30cd2d6
Compare
2347a23
to
766381e
Compare
30cd2d6
to
0ec9aee
Compare
766381e
to
b0f81a1
Compare
0ec9aee
to
6d33438
Compare
b0f81a1
to
b0c7b35
Compare
6d33438
to
d4548a9
Compare
b0c7b35
to
821c9d9
Compare
d4548a9
to
ea02cfa
Compare
ea02cfa
to
ddc0825
Compare
These individually reviewable commits consolidate some recurring patterns, move interface definitions into a separate module, adds more explanatory comments, and fixes some lingering errors.