-
Notifications
You must be signed in to change notification settings - Fork 2
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
publish message #117
publish message #117
Conversation
vladvelici
commented
May 16, 2024
- update field names for send message
- formatting
- create Message object for forwarding the message.created event
- temporary: full permissions token
- fix path to netlify functions
- add example for using publish locally
- on init messages: temporary remove history call, until we add history api
- add start-silent to have a way to not open browser window every time you start the dev server
- update dependencies
- update readme
demo/api/ably-token-request/index.ts
Outdated
@@ -30,6 +30,7 @@ Please see README.md for more details on configuring your Ably API Key.`); | |||
capability: { | |||
'room:*': ['publish', 'subscribe', 'presence'], |
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.
Can we remove this capability now, as we're not using room:
anymore?
@@ -64,25 +64,24 @@ export class Messages extends EventEmitter<MessageEventsMap> { | |||
async send(text: string): Promise<Message> { |
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 you be able to add typedocs for this file? :)
Related, I noticed |
src/Messages.ts
Outdated
return true; | ||
} | ||
default: | ||
throw new Ably.ErrorInfo(`Received illegal event="${name}"`, 400, 4000); |
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.
Not something you changed, just saw it is all, but I think the error code is wrong 👀 Should be 40,000 I think..
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.
Made this change
13e2df0
to
3f5203e
Compare
…you start the dev server
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.
Approving everything except the tests, which I added.
@splindsay-92 would you mind reviewing the tests?
It was using 4000 instead of 40000
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've added a comment, but its a small thing that probably can be left if we just want to get it merged :)
}), | ||
); | ||
}); | ||
}); | ||
|
||
describe('subscribing to updates', () => { | ||
it<TestContext>('should not miss events that came before last messages has been fetched and emit them after', (context) => | ||
new Promise<void>((done) => { | ||
it<TestContext>('subscribing to messages should work live', (context) => |
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 these tests still needed? Looks like some repeated functionality and isn't testing more than the integration tests are doing?