brianpmccullough / spfx-application-customizer-navigate

A sample spfx application customizer to test functionality related to tracking page transitions. Also highlights use of Service Locator pattern in SPFx (using ServiceScope.consume).
0 stars 0 forks source link

serviceScope.whenFinished() inside of constructor? #2

Open brianpmccullough opened 3 months ago

brianpmccullough commented 3 months ago

One thing that I am finding odd is the pattern of using serviceScope.whenFinished() callback within the constructor of the "service" class. For example:

https://github.com/brianpmccullough/spfx-application-customizer-navigate/blob/37e0203b44799f6687d3796252cb5387e4457cb5/src/services/AadApiServiceBase.ts#L26

    constructor(init: ServiceScope | AadApiServiceBaseConfiguration) {
        if (init instanceof ServiceScope) {
            init.whenFinished(() => {
                this.logger = init.consume(SPFxLoggingService.serviceKey);
                this.aadHttpClientFactory = init.consume(AadHttpClientFactory.serviceKey);
            })
        } else if (init instanceof AadApiServiceBaseConfiguration) {
            this.logger = init.logger;
            this.aadHttpClientFactory = init.aadHttpClientFactory;
        } else {
            throw new Error('Invalid instantiation parameter provided.');
        }
    }

Because whenFinished() instantiating logger (and aadHttpClientFactory) in a callback, that is being performed asynchronously. This means if the calling code (i.e. ApplicationCustomizer.ts or WebPart.ts) is not awaiting using serviceScope.whenFinished and awaiting it, there is (small) potential that a method is executed on the service class BEFORE the member variable has been initialized, resulting in need for checks to determine if the member variable has been initialized.

        this.telemetryService = this.context.serviceScope.consume(TelemetryService.serviceKey);
        this.telemtryService.trackPageView('https://www.foo.bar'); // if trackPageView simply uses logger, logger may not have been initialized.
    vs
    await new Promise<void>((resolve) => {
      this.context.serviceScope.whenFinished(() => {
        this.telemetryService = this.context.serviceScope.consume(TelemetryService.serviceKey);
        resolve();
      });
    });
    this.telemtryService.trackPageView('https://www.foo.bar'); // because we await the whenFinished() callback above, we are assured it's member variables are initialized.
brianpmccullough commented 3 months ago

I am not sure who @d-turley is, but he has some interesting stuff over here. Perhaps he/she has insights?

https://github.com/pnp/sp-dev-fx-webparts/blob/c2a8546891d5f8bd294e269537aaba8e5723953e/samples/react-rhythm-of-business-calendar/src/common/services/ServiceKeyBuilder.ts#L34

Or perhaps @tarzzi based on his code here:

https://github.com/tarzzi/enhanced-flow-command-set/blob/605869b15e187c7d867a6fa4e4bd10be44127f30/SPFx/src/di/DependenciesManager.ts#L60

tarzzi commented 3 months ago

Hi! My solution was just a quick clone and build for a coworker (forgot to remove). Original is: Cupo'365 - Enchanced flow command set.

In this solution on the line 60, the function of the ServiceKey.createCustom() is to "Constructs a new ServiceKey whose default implementation will be obtained by invoking the specified callback." MS Learn ServiceKey

Spfx refrence also says about the whenFinished method that: "To avoid mistakes, it's best to always call consume() inside a callback from serviceScope.whenFinished()." MS Learn ServiceScope And the whenFinished is indeed not an asynchronous callback. ServiceScope initialization is typically inexpensive and short lived. However, the control flow often threads through numerous constructors and base classes, which can be simplified using whenFinished().

And a great sample of the pattern over here also

But yeah I'd keep the service basically just doing one thing only, so I'd pass an ready instantiated loggerService onto the constructor of your AadApiService, so you'd have a logger ready without any additional worries about it. Also I'd get rid of the alternate AadApiServiceBaseConfiguration from the constructor, but those are my opinions 😃

Really haven't looked into this topic, (but thanks for getting me interested, will have to look a bit more😆), but hope this helps a little bit at least!

brianpmccullough commented 3 months ago

@tarzzi thank you for taking the time to have a peek and commenting here.

Regarding the AadApiServiceBaseConfiguration vs ServiceScope in the constructor, I wanted to provide flexibility in how developers could instantiate the service - either imperatively via code, or using the Service Locator (serviceScope.consume()).

Regarding "pass an ready instantiated loggerService onto the constructor of your AadApiService", is it possible to pass other constructor parameters through when you use Service Locator (i.e. serviceScope.consume()).

I think where I am particularly struggling is with the constructor having to use a callback to finalize the instantiation of it's member variables (even if "ServiceScope initialization is typically inexpensive and short lived"). For example, in the example you reference, the constructor:

    constructor(serviceScope: ServiceScope) {
        serviceScope.whenFinished(() => {
            this._aadHttpClientFactory = serviceScope.consume(AadHttpClientFactory.serviceKey);
        });
    }

The possibility that executeMyRequest() is called before the whenFinished() callback is fired/finishes.

d-turley commented 3 months ago

Years ago, I wanted to make use of the SPFx service scope, but I didn't like the API, so I tried to have the best of both worlds. I ended up building my own ServiceManager, which can initialize services asynchronously, and doesn't require callers to use the pattern of referencing a service key that itself references a static implementation of the service. It still registers the services in the SPFx service scope, but it's not really used for anything.

The issue you're running in to is also one of the reasons I wrote my own services framework.

I wrote some high-level documentation regarding my services implementation here: https://github.com/pnp/sp-dev-fx-webparts/blob/c2a8546891d5f8bd294e269537aaba8e5723953e/samples/react-rhythm-of-business-calendar/documentation/services.md