Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefer/Must use Facade notice in documentation #4

Closed
JonathanGuo opened this issue Dec 5, 2019 · 3 comments
Closed

Prefer/Must use Facade notice in documentation #4

JonathanGuo opened this issue Dec 5, 2019 · 3 comments

Comments

@JonathanGuo
Copy link

I think the documentation needs to be more clear to Laravel devs for using Facades instead of creating Kreait\Firebase\Factory.

I've encountered a caching issue by following the documentation. What I've done is created a Kreait\Firebase\Factory instance and create dynamic links.

Example:

(new \Kreait\Firebase\Factory())
            ->createDynamicLinksService($domain)
            ->createDynamicLink($url, $type);

It's working fine at my local since I don't cache my config at my local. But when I go live, this code doesn't work any more because the production runs php artisan config:cache. The Factory tries to resolve credentials through FromEnvironmentVariable::getValueFromEnvironment(). However the $_ENV is empty since I cached. If I want to use the code above I must create a ServiceAccount and attach it to the factory by withServiceAccount() method, and turn off auto discovery.

By switching to facade and then it works, which makes sense because it's facade instance is using the cached config.

FirebaseDynamicLinks::createDynamicLink($url, $type);

This library is awesome. I think the documentation could be better to remind Laravel developers MUST/PREFER using Facade to auto-discovery the service provider.

@jeromegamez
Copy link
Member

jeromegamez commented Dec 5, 2019

Thank you for letting me know, the documentation really is not clear about that (the SDK docs only refer to the SDK, not its integration).

Until I figure out better documentation on the main package, would perhaps a note in the Laravel package's README help? I was thinking about something like this in the Usage section:

You don't need and should not use the new Factory() pattern described in the SDK documentation, this is already done through the Service Provider of this package. Always use Dependency Injection, the Facades or the app() helper, otherwise the package configuration has no effect.

What do you think?

@JonathanGuo
Copy link
Author

Thank you for letting me know, the documentation really is not clear about that (the SDK docs only refer to the SDK, not its integration).

Until I figure out better documentation on the main package, would perhaps a note in the Laravel package's README help? I was thinking about something like this in the Usage section:

You don't need and should not use the new Factory() pattern described in the SDK documentation, this is already done through the Service Provider of this package. Always use Dependency Injection, the Facades or the app() helper, otherwise the package configuration has no effect.

What do you think?

Brilliant! That makes more sense to devs. Thanks. 👍

@jeromegamez
Copy link
Member

Added with bd90a20, thanks again! 🌺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants