fix: prevent circular dependency

[StenCalDabran]

Closes #261
This would be the proposed solution as discussed in the issue.

I tried to be consistent regarding test structure and so on, but I'm not sure if I was successful everywhere.
I tried creating a service for handling everything overlay-related, but due to the strong connections between the overlayRef and the dialogRef that did not work to well so for now I only extracted the "attach to overlay" functionality, i.e. the part that was responsible for the circular dependency in the first place.

The open method is still part of the ModalGalleryService, this includes no breaking changes.

[Ks89]

Hi, thank you for the PR, but now modal-gallery is not working.
Did you try the official main example to open the modal gallery component?

npm run build:lib
npm start

[StenCalDabran]

Hmm I see, I'll look into it again.

[StenCalDabran]

I finally got to looking at it again.

I had forgotten to provide the new service anywhere, so it became tree shaked and never got instantiated. Now it is provided with angular's APP_INITIALIZER token.

Tested it, everything seems to work as expected.

[Ks89]

thanks, now it's working.

I have a couple of questions:

1. Why did you add AttachToOverlayService 2 times in modal-gallery.module?

AttachToOverlayService,
    {
      provide: APP_INITIALIZER,
      multi: true,
      deps: [AttachToOverlayService],
      ...
    }

Is there a specific reason for that?

2. in attach-to-overlay.service.ts there is this method:

public initialize(): void {
    this.modalGalleryService.triggerAttachToOverlay.subscribe(payload => this.attachToOverlay(payload));
  }

but I don't seen any method to unsubscribe on close. To avoid memory leak it's a good idea to call unsubscribe. What do you think?

[StenCalDabran]

1. Basically the APP_INITIALIZER provider is just way to have a barebones implementation of something that gets the AttachToOverlayService injected. That syntax always looks a little weird, in this case the factory method also calls the .initialize() method to make it a little less awkward (the functionality would be the same if things happened in the service constructor rather than the initialize method instead and the factory method here would do nothing).
   To allow for the possibility of the AttachToOverlayService being injected it still needs to be provided, which happens in the first line (instead it could be part of an imported Module but that does not match the situation here).
   -> first line provides, the other lines take and use it.
2. Since the service is providedIn: 'root' and setup at APP_INITIALIZE time, there will only ever be this single instance running and it should be available for as long as the application is there. With the app shutdown, the ressources will be freed anyway, so there is no functional difference. I can still implement the OnDestroy lifecycle hook if you want, but that will only ever become relevant if the provisioning should be changed from providedIn: 'root' to something else.

[Ks89]

Ok, it's clear.

I made a quick test removing AttachToOverlayService and leaving only

{
      provide: APP_INITIALIZER,
      multi: true,
      deps: [AttachToOverlayService],
      ...
    }

and it's working. This was the reason for my first question. AttachToOverlayService is already defined with providedIn: 'root'.

[StenCalDabran]

Oh, makes sense, as a service it has not to be provided again. My bad :-).

[Ks89]

You forgot to remove that import import {ModalGalleryComponent} from './modal-gallery.component'; to remove the circular dep, but It's ok, I'll apply the fix by myself.

I'll integrate also madge as a CI step to prevent circular deps in the future.

Thank you for your contribution.

[Ks89]

@StenCalDabran I saw that my CI has an issue, because it didn't report an error on npm test.
There is a test that is failing, I also reproduced the same error on my local machine. I'm trying to understand how to fix it, before releasing a new version.

PS: I fixed the CI, now it's reporting the error

[StenCalDabran]

You forgot to remove that import import {ModalGalleryComponent} from './modal-gallery.component'; to remove the circular dep

Well, that's embarassing 😳

Thank you for the fix!

[Ks89]

No problem man.
My broken CI is even more embarassing 😁

However, I can't fix the broken test so easely. I understood the idea, but there Is something broken. If you want to help me, I'm open to a solution. The broken Is test Is the attach service.