nuxt-community / firebase-module

🔥 Easily integrate Firebase into your Nuxt project. 🔥
https://firebase.nuxtjs.org
MIT License
640 stars 98 forks source link

Feat: load config from external file #325

Closed MoeFarag closed 3 years ago

MoeFarag commented 3 years ago

Adds the ability to setup the Firebase project config through an external file and thus processed during run-time rather than build time. Gives flexibility in how the config happens and deployed in different environments; this is especially relevant when deploying the application in a container

MoeFarag commented 3 years ago

@lupas Hope you can review this PR when you can

lupas commented 3 years ago

@MoeFarag Thanks for the PR and sorry for the late response. Cool idea, never thought of that!

Quite busy this week, I'll try to find time to properly look at it before Sunday.

I wanna finish up https://github.com/nuxt-community/firebase-module/pull/258 by the end of this week - since it changes the entire codebase drastically - and would then merge this in afterwards if all looks good.

Then we could ship this with v7.0.0 which is up for release soon (hopefully next week).

Hope that is fine for you & thanks again!

MoeFarag commented 3 years ago

@lupas thanks for the reply. Yeah just saw how big #258 is. Good luck with that.

Take your time looking at it but would appreciate including it in the v7.0.0 so we can use it right away. Let me know if you need anything else.

btw, the idea was inspired by the apollo plugin actually (so thanks to that) + a blocker on my side.

MoeFarag commented 3 years ago

@lupas how's this PR looking? any updates on release date for v7.0.0

lupas commented 3 years ago

@MoeFarag Just merged https://github.com/nuxt-community/firebase-module/pull/258 and will now take a look at this one and hopefully be able to merge it today or otherwise in the coming days.

Then I plan to do some other improvements & solve some issues before releasing v7.0.0, but I'd say in a couple of days I should be able to finish release it. Fingers crossed :)

lupas commented 3 years ago

Hey @MoeFarag

Quickly had a look at the PR, some remarks:

1) Loading from an external file is not really an issue, since this is already possible by using require() or await import() within nuxt.config.js.

2) The runtime aspect however is very interesting. But since you are just using require, the only option to change the config during runtime is to actually change the config file locally on the server. Is this really your approach - what use case would this be? Wouldn't it make more sense to use await import('../config.js')? That way we would be able to load config asynchronously from an external source, like even another webserver.

3) We would also need to mention in the docs that the Auth SSR Option as well as the Messaging createServiceWorker option are not working with this, since we inject the config on build time into the service-workers.

If we find a working solution that can also load the config asynchronously, I think this would be a really cool addition.

Unfortunately I'm out of time for today, I hope I can do some tests during the week. br, Pascal

MoeFarag commented 3 years ago

Thanks for reviewing @lupas

1) We'll that is possible, this what the addition actually does,, but I haven't seen that being used in the config files. Most configurations that require an external path, just reference a file path, like in Apollo, and setting Loading component. Adding this makes it a feature of the library not a work around with require. 2) No the notion is not to change the content of the files but rather to grab it from through other means, Env Vars, backend calls, making it conditional on any case.. At the end of the day the file is JS and gets the entire Nuxt Context so logic and possibilities are endless, it is won't be just just an export default const config in which case just importing does it. In my use case, I use environment variables passed down through the $config object injected as part of the Nuxt Context. So the dependency on the Firebase setting is runtime rather than build time 3) Probably missed that, but we can check if it is possible to enable the feature for them as well it makes sense.

MoeFarag commented 3 years ago

@lupas any reply on the above,, shall I check the conflict and resolve or we need further discussion regarding the need for the feature?

lupas commented 3 years ago

@MoeFarag

Sorry for the late response, I had a bunch of other things on my hand the last few days. Decided to finish up Version 7 first because the new Firebase v8 didn't work with v6, so I wanted to get that release out as quickly as possible.

I wanted to merge & resolve the conflicts from the current dev branch but don't have permissions to push to this branch, could you allow permissions for contributors?

I will then merge dev in and resolve the conflicts, and probably do some tests and changes from my side as well.

Would be great if we can figure out a way to make it work with SSR & the service-workers, too.

MoeFarag commented 3 years ago

No worries

lupas commented 3 years ago

@MoeFarag Thanks! I merged dev into the branch - so the branch now works with Version 7.

Open Points:

lupas commented 3 years ago

Hey @MoeFarag Still something you want to work on, or should I close this PR?

So far I haven't come across a single case who would require this PR, therefore I would suggest we close it to make the module not more complex for not frequently used functionality.

Thx for a short update.

MoeFarag commented 3 years ago

@lupas sorry for the late reply. Sorry I got swamped with other stuff. Fine lets close it for now and I'll try to do it at a later stage