[WIP] Workaround fix for the init() being invoked twice.

[nkmdk-007]

Problem Whilst creating the AccountUIComponent and testing it's initialisation, I've noticed that the componentLoader creates the Components correctly (invokes Constructor function once) but the second loop seems to invoke the Components.init() function twice which could result in memory leaks.

The problem seems to be with the RSVP promise code, perhaps an issue of implementation? I'm not sure. Having a debugger statement in the allSettled block shows this is called twice and therefore the Components are initialised twice and therefore firing two INITIALISE-SUCCESS events for each deferred Component.

Solution At the moment the workaround here is to set an initialised flag inside the DoughComponent which is set from this._initialisedSuccess() which is, by convention, to be called when the init() function has finished. We can then check for this flag in the componentLoader prior to calling init()...

N.B. This is a workaround until we have the time to spend more time working out how the componentLoader is using Promises.

[davidtrussler]

I'm a little concerned about putting this change in right now. Although it's a small change it could have major ramifications on all components that use the Dough Component Loader. I think we should raise a ticket for this issue and deal with it separately from this feature.

[nkmdk-007]

OK, perhaps we can talk this one through a bit more. Without this patch the init() function is called twice for each deferred Component that is instantiated.

The fix is to ensure the init() is only called once and in my tests it doesn't affect any existing Components.

[davidtrussler]

@nmiddleweek I guess it depends a bit on if this is critical to this stream of work or not. It just looks to me like the kind of change that could have unintended consequences so is best left to a separate piece of work if we can live with it for now. Either way there should be a ticket for it. Happy to defer to other team members on this though. Any thoughts @willhall88 @rhianwatkins1 @jiveturkeyJay?

[rhianwatkins1]

I agree, there should be a ticket raised, and tests preferably. This would definitely need properly testing and qa can't test without a ticket. Why is init being called twice in the first place? If it's because the component is getting initialised twice then surely this is not going to stop that. I think the problem should be addressed rather than a workaround.

[willhall88]

I noticed this behaviour recently as well, but didn't have time to go down the rabbit hole. @nmiddleweek, can the architecture tickets be completed without this patch being implemented?

I'd definitely prefer a ticket to be created to investigate this further and understand, why and where this issue is occurring (i.e. on all tools/dough implementations or just pacs). i.e. the full solution you allude to rather than this workaround. I'd like to make sure we do everything we can to complete the architecture work this sprint before the code freeze and so if we can look into this after the other work I'd prefer it, just because I'm not clear of the implications for how much testing and investigation is needed.

[willhall88]

Because this gem is updated on the frontend, there could be complications we miss elsewhere on the mas estate and so we need a thorough testing plan before proceeding

[nkmdk-007]

@nmiddleweek I guess it depends a bit on if this is critical to this stream of work or not. It just looks to me like the kind of change that could have unintended consequences so is best left to a separate piece of work if we can live with it for now. Either way there should be a ticket for it. Happy to defer to other team members on this though. Any thoughts @willhall88 @rhianwatkins1 @jiveturkeyJay?

@davidtrussler, @willhall88 - The alternative workaround is to put similar code into the specific components that are being deferred. Immediately this is the AccountUIComponent and in the very near future this will likely extend to the ReadMore, RelatedFees and Icon components.

The init functions are only meant to get invoked once and the fix in the PR ensures that by implementing a property at the component level called this.initialised. As per the PR description, this is related to how the componentLoader is using the Promises library. For some reason deferred components are having their initialised function invoked twice. Admittedly I am proposing a patch to the solution however that patch will either need to be applied at the Dough Core or at the specific Dough Component level.

I haven't come across this issue before and it's likely because the MAS estate doesn't use deferred components... https://github.com/search?q=org%3Amoneyadviceservice+data-dough-defer&type=Commits

I noticed this behaviour recently as well, but didn't have time to go down the rabbit hole. @nmiddleweek, can the architecture tickets be completed without this patch being implemented?

@willhall88 - Can you remember where you have seen this?

I agree, there should be a ticket raised, and tests preferably. This would definitely need properly testing and qa can't test without a ticket. Why is init being called twice in the first place? If it's because the component is getting initialised twice then surely this is not going to stop that. I think the problem should be addressed rather than a workaround.

It does prevent it, it uses a property to prevent the init() function from being called twice.

Because this gem is updated on the frontend, there could be complications we miss elsewhere on the mas estate and so we need a thorough testing plan before proceeding

OK, I will implement the patch at the individual Component's level instead of a generic patch.