-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
cc @wagnerjt for reviews on service changes |
Not ready until I add tests for the driver... :) |
done. @stellasia, @wagnerjt I'd love your reviews on pr |
@stellasia, let's review and close this PR when possible. |
Sure, will do during the coming week! |
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 found a couple of other bugs when reviewing but not related to this PR. So looks good to me when the issue with records
is fixed (see comment in code)
Thank you very much for this @JoDoM , merging 💯 |
A singleton neo4jService that encapsulates driver. Layers don't need to have a knowledge of underlying neo4j driver. Composition proposed here allows us to smoothly swap out drivers in future #65, and remove driver instantiation on critical boot path #78.
Additionally, this PR formalizes a result API on neo4jService endpoints. Success from service calls return as
{ status: 200, result: someResult }
while failures return{ status: 500, error: errorWithStack }
Next: This pattern opens up an opportunity for better error ux, most immediately, a path for solving #77 [ don't rerender if we hit error on create/update new layer ]
PS: Builds on top of changes in PR #75