cloudcreativity/laravel-json-api

Make StoreInteface, ResolverInterface and ContainerInterface singleton

zlodes opened this issue · 3 comments

zlodes commented

Hi!

I think that StoreInterface, ResolverInterface and ContainerInterface must be registered as singleton.

The current implementation allows to get e.g. ContainerInterface from Laravel Container multiple times and each call will fire the bind callback.

Another problem is calling after resolving callbacks. Each after-resolving callback will be called multiple times:

$this->app->afterResolving(ResolverInterface::class, static function (ResolverInterface $resolver): void {
    // Do something with the Resolver
});

From ServiceProvider:

$this->app->bind(StoreInterface::class, function () {
    return json_api()->getStore();
});

$this->app->bind(ResolverInterface::class, function () {
    return json_api()->getResolver();
});

$this->app->bind(ContainerInterface::class, function () {
    return json_api()->getContainer();
});

I suggest to change bind call to singleton.

I can make PR if the idea will be approved.

lindyhopchris commented

Hi! So they are already singletons as they are cached on the Api object that they are resolved from, which is why I've used bind in the service container.

I'm not sure if there would be any unintended consequences of switching the container bindings to singletons. Feel free to submit a PR - if none of the tests fail then I'd probably accept it, though I'd probably wait for a major release (rather than a minor) to err on the side of caution.

  • 👍1
zlodes commented

@lindyhopchris PR #590 has been created. Tests passed. 👌🏻

lindyhopchris commented

@zlodes thanks for the PR, I've now merged with develop. I'm going to tag as a minor release (will be 3.4.0) but won't do that immediately as it's currently the only change sitting on develop.

If you want to use it now, set your minimum stability to dev then:

composer require cloudcreativity/laravel-json-api:^3.4

There's a branch alias so that'll pull in develop with your changes.

  • ❤️1