Enterprise-ng: Modal: Remove duplicate services

[tmcconechy]

Describe the bug
There is now two modal services. We should only have one to make it easier to support things. Such as adding new API functions/settings. Fixing bugs.

1. https://github.com/infor-design/enterprise-ng/tree/master/projects/ids-enterprise-ng/src/lib/modal
2. https://github.com/infor-design/enterprise-ng/tree/master/projects/ids-enterprise-ng/src/lib/modal-dialog

Try to do this for 7.0 release.

Expected behavior
We just have one API to support. Since SohoModalService is newer and likely less in use we can remove it?

Questions

• Anyone use this?
• How do we verify the Memory leaks are fixed in SohoModalDialog?

[tmcconechy]

cc @nbcp @bthharper @pwpatton @fitzorama

[nbcp]

I thought that for the 7.0 release, since it is a major upgrade, we are going to deprecate SohoModalDialogModule and have SohoModalModule to replace it instead?

I am currently in the process of migrating several of landmark's modals to use the newer one because the old one doesn't fix memory leaks of components created inside the modal. Aside from that, the old modals are also still prone to leaks when using vetoable event guards.

[tmcconechy]

So, Then I'm not sure. @bthharper was making it seem to me like the SohoModalDialog did not seem to have any leak issues. But we do have https://github.com/infor-design/enterprise-ng/issues/508

I do see now that the options are shared so that's good. I dont particularly care which one but breaking changes are not prefered. So I would suggest:

• make a decision which one?
• add better notes that one is deprected - do not remove it right away - update the change log
• if one or the other fixes https://github.com/infor-design/enterprise-ng/issues/508 that would be the best situation.
• update the example to only show "the one"

What you all think?

[bthharper]

I am not aware of any memory leaks in the original modal dialog, and it still provides the veto support we use. Not both the original and the newer modal have the issue reporting in #508.

[bthharper]

Just did a quick check, and I can't see any memory leaks in the original mode code.

[tmcconechy]

@nbcp would you test again on master? I think that https://github.com/infor-design/enterprise-ng/pull/784 solves some of the veto leaking issues as now he turns off the events and closes and destroys on page change now.

So if so not seeing a value to a new duplicate component. It doesnt have that fix with url change and is already out of sync. Would save some migration.

If any more issues on SohoModalDialog perhaps we can refocus there. If still there.. can we construct an issue that explains how and under what situations we see the memory leak? And how to test for it ect..?

For now i commented out the example for SohoModal so it doesnt get out there too much and we can release.

[nbcp]

Sorry for the late reply,

@tmcconechy I used "Full on Tablet" on the modal-dialog demo and tested opening the modal a few times. FullSizeModalDialogComponent objects are leaking as well as LComponentView_FullSizeModalDialogComponent.

However, testing it using SohoModalService still also leaks those objects. This might be caused by Angular 9 but I'm not sure. This gets me worried because if the component inside the modal is leaking, everything in it would still continue to run even when the modal is closed already. @bthharper how did you check on your end?

[bthharper]

@nbcp - it's interesting you say that as I had similar results, initially. The only component that leaked was the first one (FullSize). I played around a bit, and then it stopped leaking. Oddly, none of the others leak.

There does seem to be an angular 9 change, as there's no other changes. I'll take another look.

[nbcp]

@bthharper what makes FullSize different form the others is it is using the apply() method which gives it access to the component inside the modal. Which is very essential and a common use case as it is the only way right now to feed inputs into the component.

[tmcconechy]

Still thinking just as an update we see if the core component has any memory leaks and fix those. Wonder if the NG one would tnen fix, or if its insurmountable?

Either way still thinking we remove the new component in favor of fixing the currently used one.

[tmcconechy]

Related memory leak issue https://github.com/infor-design/enterprise/issues/3785

[bthharper]

@nbcp - sorry meant to say I'd removed all differences and still only the FullSize leaked. It's odd.

    const dialogRef = this.modalService
      .modal<FullSizeModalDialogComponent>(FullSizeModalDialogComponent, this.placeholder)
      .buttons(
        [
          { text: 'Cancel', click: () => { dialogRef.close('CANCEL'); } },
          { text: 'Submit', click: () => { dialogRef.close('SUBMIT'); }, isDefault: true }
        ])

      // Don't close this dialog on navigation
      // .closeOnNavigation(false)
      // .title(this.title)
      // .isAlert(this.isAlert)
      //.apply((dialogComponent) => { dialogComponent.model.header = 'Header Text Update!!'; })
      .open()
      // .afterClose(result => {
      //  this.closeResult = result;
      //}) 
     ;

[bthharper]

@tmcconechy - that related issue looks interesting, as all the relevant angular destroy code is being called, just not cleaning up.

[volante007]

Just some additional input from the Homepages / Widgets team.

A _lot_ of widgets are using SohoModalDialogService & SohoContextualActionPanelService. Removing those services would cause a lot of issues, and teams having to rewrite their widgets. Widgets need to be able to rely on "stable" APIs / services.

100+ widgets from 30+ teams use IDS angular in their widgets, every breaking change demands a lot of effort to get them working before the new Infor OS release is out in production (and their widgets would fail without code changes)

[tmcconechy]

I agree. I do not think we should add the new Modal. We should fix the old one. As of now its in there but not sure if its any better. This ticket is to remove / pull out the new one.

[tmcconechy]

So now with the latest changes in (the older modal). Is everyone cool if we drop the new stuff out now for next release?

[nbcp]

@tmcconechy I'm cool with it, but can we also get this in please: #813.
It's just a readme change plus switching the parent parameter to optional since it's not really used.