-
Notifications
You must be signed in to change notification settings - Fork 94
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
migrate some legacy events #3246
base: master
Are you sure you want to change the base?
Conversation
14ea030
to
a307d94
Compare
@thisconnect @bznein I continued adding more commits to this PR migrating more events, resulting in a much bigger PR. Let me know if I should split it into multiple PRs or if it is manageable. |
0e1fbc5
to
4a380c3
Compare
For me it's fine, I'll do only code review for now and when you done adding new commits, test everything in one go. |
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.
very nice 👍
did some frontnend review, untested LGTM (added some questions).
But I'll wait for backend review and will do some testing at the end.
}; | ||
}; | ||
|
||
apiWebsocket(handleMessages); |
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.
🎉
backend/devices/bitbox02/device.go
Outdated
@@ -73,12 +74,31 @@ func NewDevice( | |||
log: log, | |||
} | |||
device.Device.SetOnEvent(func(ev firmware.Event, meta interface{}) { | |||
// Old-school | |||
device.fireEvent(event.Event(ev)) |
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.
is the old-school (event?) here still needed?
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 so b/c of bitbox01. I might try to remove that later.
Edit: Oh wait this is bitbox02/device.go, not backend.go. Maybe not needed, I'll check.
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.
You were right, all the events were converted, so I removed the old-school event here.
98202bd
to
ee44b20
Compare
It is only used for BitBox02. We submit the status in the Object so the frontend can skip a `getStatus()` call.
Easier structure, to have all bb02 api calls in one file. E.g. the statusChanged subscription needs `TStatus`.
d7c9011
to
7d55ee2
Compare
In 3d70ed2, we removed keystore support for BitBox01. As a result this event and all dependents are not used anymore.
Use status directly as passed, and inline the function.
Was only used in bitbox01.jsx, and there it was actually useless since DeviceSwitch is fed the devices list already (from app.tsx using the new style event) to decide to mount/unmount the component.
Tagged both @thisconnect and @bznein for frontend and backend, and @bznein especially as maybe it touches on his current work related to removing the synchronizer blocks.