-
Notifications
You must be signed in to change notification settings - Fork 1
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
William/v2 prototype #40
base: master
Are you sure you want to change the base?
Conversation
d89f1fc
to
e030330
Compare
The v2 API passes the key in the payload.
e030330
to
615dddd
Compare
export const asPushMessage: Cleaner<PushMessage> = asObject({ | ||
title: asOptional(asString), | ||
body: asOptional(asString), | ||
data: asObject(asString) |
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.
Isn't data
optional?
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.
Some feedback to discuss
export type PushEventState = | ||
| 'waiting' // Waiting for the trigger | ||
| 'cancelled' // Removed before the trigger happened | ||
| 'triggered' // The trigger happened, but not the effects | ||
| 'complete' // The trigger and effects are done | ||
| 'hidden' // Removed after being triggered |
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.
Are cancelled
and hidden
internal states? If a client removes a push event it is not returned by the public API, but it remains in the DB with either of these states?
If I understand correctly, triggered
is the state just before completing the event (so this is also an internally-concerned state). I think we should word this as // The trigger and event is done
, because at least on the client-side, the word "effect" is the derived state(s) from the action/event; and the action/event is the cause of the effect.
export interface NewPushEvent { | ||
readonly eventId: string | ||
readonly broadcastTxs?: BroadcastTx[] | ||
readonly pushMessage?: PushMessage | ||
readonly recurring: boolean | ||
readonly trigger: PushTrigger | ||
} |
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.
Doesn't this belong in the pushApiTypes.ts
?
|
||
const apiKey = 'demo-api-key' | ||
const deviceId = 'example-device' | ||
const loginId = base64.parse('EE+tBb5wM63qwCDVidzwUQThH9ekCSfpUuTQYujSmY8=') |
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 this loginId a test loginId?
/** | ||
* All push server HTTP methods use "POST" with JSON. | ||
*/ | ||
async function postJson(uri: string, body: unknown): Promise<unknown> { |
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 wish I had noticed this function early. I like it and would prefer to use it in the GUI (if not now, then later).
await postJson( | ||
'http://127.0.0.1:8001/v2/device/update/', | ||
wasPushRequestBody({ | ||
apiKey, | ||
deviceId, | ||
data: wasDeviceUpdatePayload({ loginIds: [loginId] }) | ||
}) | ||
) |
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 would be the isomorphic dream:
import pushServer from 'https://push.edge.app/api'
pushServer.device.update({ apiKey, deviceId, data: { loginIds: [loginId] } })
This magic would be enabled with URL imports in our client-side environments (something Deno or Bun enables) and using a special HTTP server framework (ala serverlet) which could syndicate a public API derived from the defined routes at the /api
route.
TL;DR: Another thing to add to the side-project/experiment list.
) | ||
} catch (error) { | ||
if (asMaybeConflictError(error) == null) throw error | ||
await addEvent(connection, event, new Date(created.valueOf() + 1)) |
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.
Would this recursion thrash? The created date param wont prevent the recursive call to no longer conflict. We may want a delay here?
/** | ||
* Handles all the effects once a row has been triggered. | ||
*/ | ||
export async function triggerEvent( |
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.
Where is this used? Or is it used in a later PR?
Almost everything should be here now from the server point of view, but it needs to be tested, and we need the publisher, rates checker, and blockchain checkers still.