Skip to content
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

Enable signaling of bridge errors #111

Open
wants to merge 19 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@V02460
Copy link
Contributor

commented Jun 13, 2019

Adds the ability of signaling permanent errors occuring at the bridge to the SDK as well as automatic sending of these events. If the bridge rejects a room event that was handed to it, the room will automatically be notified of this error.

Mainly two things are added by this PR:

  • A new method signalBridgeError is added to the Intent class.
  • Rejection of requests provided to the onEvent callback will trigger the new intent.

Bridges should not have to do much work or work at all to profit from the new feature. Every bridge rejecting the request object delivered to it (as should be the case already) will profit. If a bridge swallows errors and simply logs them, code has to be changed to reject the request object properly.

In addition to these changes there is some refactoring and documentation going on, mainly related to the EventQueue class.

@Half-Shot

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

Thanks for submitting the PR. Could you give a rough changelog in the PR description for the viewers at home?

@Half-Shot
Copy link
Contributor

left a comment

This looks like it's generally going in the right direction 👍 . I have more feedback but I think if we fix these first it will be eaiser to review the rest. Also thank you for doing tests!!!

Show resolved Hide resolved lib/bridge.js Outdated
Show resolved Hide resolved lib/bridge.js Outdated
Show resolved Hide resolved lib/bridge.js Outdated
Show resolved Hide resolved lib/bridge.js Outdated
Show resolved Hide resolved lib/bridge.js
Show resolved Hide resolved lib/bridge.js Outdated
Show resolved Hide resolved lib/bridge.js Outdated
Show resolved Hide resolved lib/errors.js Outdated

@V02460 V02460 marked this pull request as ready for review Jun 14, 2019

@V02460

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

Changed PR from “draft” to “ready for review” as Travis CI does currently not work with draft PRs.

V02460 added some commits Jun 12, 2019

Create subtypes for EventQueue
Signed-off-by: Kai A. Hiller <V02460@gmail.com>
Tidy up usage of EventQueues
Signed-off-by: Kai A. Hiller <V02460@gmail.com>

@V02460 V02460 force-pushed the V02460:reliable branch from f5fbf9d to a91937b Jun 15, 2019

V02460 added some commits Jun 13, 2019

Add error classes for bridge errors
Signed-off-by: Kai A. Hiller <V02460@gmail.com>
Add intent to signal bridge error
Signed-off-by: Kai A. Hiller <V02460@gmail.com>
Remove prolong function for simpler alternative
Signed-off-by: Kai A. Hiller <V02460@gmail.com>

@V02460 V02460 force-pushed the V02460:reliable branch from a91937b to e173ab1 Jun 16, 2019

V02460 added some commits Jun 16, 2019

Move EventQueue to own file
Signed-off-by: Kai A. Hiller <V02460@gmail.com>
Add JSDoc annotations to event-queue.js
Signed-off-by: Kai A. Hiller <V02460@gmail.com>
@Half-Shot
Copy link
Contributor

left a comment

Few more things, but generally good enough to show a working example of.

Show resolved Hide resolved lib/components/intent.js Outdated
Show resolved Hide resolved lib/components/intent.js Outdated
Show resolved Hide resolved lib/components/intent.js
Show resolved Hide resolved lib/components/intent.js Outdated

V02460 added some commits Jun 22, 2019

Fix perRequest setting for queues
Signed-off-by: Kai A. Hiller <V02460@gmail.com>
Fix eslint warnings
Signed-off-by: Kai A. Hiller <V02460@gmail.com>
Add logic for reacting to permanent errors
Signed-off-by: Kai A. Hiller <V02460@gmail.com>
Change bridge error affected_users to use a regex
Signed-off-by: Kai A. Hiller <V02460@gmail.com>
Fix jsdoc comments
Signed-off-by: Kai A. Hiller <V02460@gmail.com>

@Half-Shot Half-Shot self-requested a review Jun 28, 2019

@V02460 V02460 force-pushed the V02460:reliable branch from c87f79a to dcd323e Jul 4, 2019

@Half-Shot
Copy link
Contributor

left a comment

Looking very close for a release :)

Show resolved Hide resolved spec/unit/intent.spec.js Outdated
}

module.exports = {
wrap,

This comment has been minimized.

Copy link
@Half-Shot

Half-Shot Jul 17, 2019

Contributor

wrap might be too generic a name :/

This comment has been minimized.

Show resolved Hide resolved lib/errors.js Outdated
Show resolved Hide resolved lib/errors.js Outdated
Show resolved Hide resolved lib/errors.js Outdated
Show resolved Hide resolved lib/bridge.js
Show resolved Hide resolved lib/bridge.js
Show resolved Hide resolved lib/bridge.js
Show resolved Hide resolved lib/bridge.js Outdated
Show resolved Hide resolved lib/bridge.js Outdated
@Half-Shot

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

So this all looks good after #111 (comment) gets resolved. Can we get a quick how-to use section in README.md like we do for other components so other bridge developers can understand? This should supplement the jsdoc

Change type of bridge error event
Signed-off-by: Kai A. Hiller <V02460@gmail.com>

@V02460 V02460 force-pushed the V02460:reliable branch from 62fee2b to ee4ff82 Jul 19, 2019

@Half-Shot
Copy link
Contributor

left a comment

README looks largely fine but I have another behavior concern (sorry)

Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved README.md Outdated

Kai A. Hiller and others added some commits Jul 19, 2019

Kai A. Hiller Kai A. Hiller
Make EventQueue compatible with ES Promises
Signed-off-by: Kai A. Hiller <KaiAlexHiller@web.de>
Beautify
Signed-off-by: Kai A. Hiller <V02460@gmail.com>
Add TODO related to bridge error sending
Signed-off-by: Kai A. Hiller <V02460@gmail.com>
Add README section about bridge error signaling
Signed-off-by: Kai A. Hiller <V02460@gmail.com>

Kai A. Hiller added some commits Jul 19, 2019

Kai A. Hiller Kai A. Hiller
Correct bridge error text in README
Signed-off-by: Kai A. Hiller <KaiAlexHiller@web.de>
Kai A. Hiller Kai A. Hiller
Make _onEvent async
Signed-off-by: Kai A. Hiller <KaiAlexHiller@web.de>

@V02460 V02460 force-pushed the V02460:reliable branch from b0e6c35 to 7c7d5b2 Jul 19, 2019

@Half-Shot

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

Looks like there are a few linting warnings we can clean up, and istanbul is not pleased about our async syntax :(

@V02460

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

I cleaned up linter warnings in all files I touched and did #116 for the issue with Istanbul.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.