feat: connectWithRetry for amqp; + amqp package#200
Conversation
|
I think this is an interesting idea in general. I'm concerned that these packages won't work, as currently created, in the nr-app. I just pushed a big change that reworks how nr-common is imported in the different packages, so now it doesn't have the build folder committed. It seems to be working so far, at least locally and the builds now worked. My sense is that it gets complicated to have multiple shared packages. For example, the GitHub actions are configured to run whenever anything in their directory or nr-common changes, if we have multiple directories, we'd have to maintain that mapping. My first thought would be to abandon this for now, and explore it next sprint at the beginning of the week rather than the end, so we have some time to fix the issues. I'm also not sure that it's going to be a net win right now. It seems like a nice optimisation, but will add complexity and unexpected issues now, for gains later in terms of better separation of concerns and so on. |
This is sort of a proposal for how we could have smaller shared packages, because nr-common is in my opinion quite crowded and more modular packages can help us to figure out better abstractions down the line. It keeps things related to a domain bundled together and allows for more clear dependency scoping, no need for a build or publish for that, etc.
Can be improved upon but generally wondering if this is something we might want. I do it like this on plenty of projects.
As an example, it fixes an issue where the validations server would not be able to recover when the amqp connection fails, and it's ported to all the other apps using it.