fix: listen to incoming messages on agent initialize not constructor

[niall-shaw]

No description provided.

[codecov-commenter]

Codecov Report

Merging #1542 (5f3f8fc) into main (0f528ba) will not change coverage.
The diff coverage is 60.00%.

@@           Coverage Diff           @@
##             main    #1542   +/-   ##
=======================================
  Coverage   79.81%   79.81%           
=======================================
  Files         917      917           
  Lines       22150    22150           
  Branches     3888     3888           
=======================================
  Hits        17679    17679           
  Misses       4160     4160           
  Partials      311      311           

Files Changed	Coverage Δ
packages/core/src/agent/Agent.ts	93.22% <60.00%> (ø)

[genaris]

Thanks @niall-shaw ! I don't think it was done on purpose. Agent objects are usually initialized only once during their lifecycle. With this fix were you able to make several shutdown / initialize sequences?

My only concern to merge it right now is about the fact that messageSubscription is for some reason a public field, so making it optional could be considered a breaking change. However, I don't know in which use-cases this field is actually accessed from outside. What do you think @TimoGlastra (Git blames you 😛)?

[niall-shaw]

@genaris

With this fix were you able to make several shutdown / initialize sequences?

Yes I was :)

[niall-shaw]

@TimoGlastra @genaris - any updates?

[TimoGlastra]

However, I don't know in which use-cases this field is actually accessed from outside. What do you think @TimoGlastra (Git blames you 😛)?

Yeah not sure why this is public. I think we can merge without it being a breaking change and see this as a fix.

[niall-shaw]

FYI @TimoGlastra - on AFJ WG call, it was mentioned by both @genaris and @berendsliedrecht that this may be considered breaking change. I was going to look into a different solution. What do you think?

[TimoGlastra]

Yeah it's definitely a breaking change, but I'm not sure why anyone would use this property, and thus I'm okay with merging it as is, as it should never have been exposed.

But other solutions also fine for me

[genaris]

Yeah it's definitely a breaking change, but I'm not sure why anyone would use this property, and thus I'm okay with merging it as is, as it should never have been exposed.

But other solutions also fine for me

Yeah @niall-shaw I think it'd be better to make this field private and treat it as a fix to not overcomplicate things. Then we merge it for 0.4.1.