Why is firebase a dependency?

frsechet commented 6 years ago

Hello,

I just saw this warning in the console:

It looks like you're using the development build of the Firebase JS SDK.
When deploying Firebase apps to production, it is advisable to only import
the individual SDK components you intend to use.

For the CDN builds, these are available in the following manner
(replace <PACKAGE> with the name of a component - i.e. auth, database, etc):

https://www.gstatic.com/firebasejs/5.0.0/firebase-<PACKAGE>.js

Which made me wonder: why would firebase (and related fcm package) be imported here, if pushjs itself doesn't need it and this is supposed to be a thin ember wrapper around it? Is there anything you need firebase for, and in that case, why is it not mentioned in the documentation?

Among other reasons I see not to include it:

one more dependency to manage
firebase belongs to Google which may not be everyone's cup of tea
https://cost-of-modules.herokuapp.com/result?p=firebase@5.3.1

I just tried removing firebase completely and there doesn't seem to be any bug. Would you accept a PR that removes firebase completely?

frsechet commented 6 years ago

Hi @devotox,

did you get a chance to review #27 ?

It has been running in production on our stack for about a month now without any issue. Hope you can merge and release soon! How can I help?

frsechet commented 5 years ago

Hi @devotox, do you need help with maintaining this repo? Would love to help.

devotox commented 5 years ago

Yes would love help maintaining. And I believe firebase is a dependency since you can use the gcp push messaging with this addon

On Mon, 17 Dec 2018, 16:27 Francois Falala-Sechet <notifications@github.com wrote:

Hi @devotox https://github.com/devotox, do you need help with maintaining this repo? Would love to help.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/devotox/ember-cli-pushjs/issues/26#issuecomment-447906823, or mute the thread https://github.com/notifications/unsubscribe-auth/AB-BzszimrwiHTrVVE-Evi6CPqTalXEsks5u58X-gaJpZM4V86Jz .

frsechet commented 5 years ago

Cool.

I would suggest making 2 different plugins then, because it does not fit the description (and most common need) of a Simple Wrapper around Push JS.

one plugin is the wrapper around pushjs
another plugin is built on top of this one for people who want to use firebase (and proposes a boilerplate for other push service integrations)

Reasoning is that firebase is not the only provider of push notification that exists, while pushjs is "how" the notifications are technically displayed. If people want to integrate their own notification service, they can use the basic pushjs and add the service on top.

The cost of forcing firebase onto everyone is simply not worth it as it is not at all universal! 🙂

devotox commented 5 years ago

I think you are definitely correct.

I was using firebase heavily when I created this plugin but now I believe it should be a lot more generic like you propose.

Are you able to make a pull request doing just this?

On Thu, 20 Dec 2018, 11:46 Francois Falala-Sechet <notifications@github.com wrote:

Cool.

I would suggest making 2 different plugins then, because it does not fit the description (and most common need) of a Simple Wrapper around Push JS.

one plugin is the wrapper around pushjs

another plugin is built on top of this one for people who want to use firebase (and proposes a solution

Reasoning is that firebase is not the only provider of push notification that exists, while pushjs is "how" the notifications are technically displayed. If people want to integrate their own notification service, they can use the basic pushjs and add the service on top.

The cost of forcing firebase onto everyone is simply not worth it as it is not at all universal! 🙂

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/devotox/ember-cli-pushjs/issues/26#issuecomment-449061993, or mute the thread https://github.com/notifications/unsubscribe-auth/AB-BzhAafcQVXyB1usxCblFXnoVCyUArks5u677zgaJpZM4V86Jz .

frsechet commented 5 years ago

How about that: https://github.com/devotox/ember-cli-pushjs/pull/27 🙂

I've been running that in production for a few months now and haven't gotten any issues, so it should be safe.

However, IMO, this should be a major version bump, as this effectively deprecates (undocumented, and probably for that reason, unused) a large functionality of this plugin.

frsechet commented 5 years ago

Thanks for the merge!

Don't forget to release to npm ;-) And let me know if you need further help!

frsechet commented 5 years ago

@devotox can you release to npm please?

devotox commented 5 years ago

Haaaa yes will do on the next few days

On Fri, 28 Dec 2018, 16:37 Francois Falala-Sechet <notifications@github.com wrote:

@devotox https://github.com/devotox can you release to npm please?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/devotox/ember-cli-pushjs/issues/26#issuecomment-450431430, or mute the thread https://github.com/notifications/unsubscribe-auth/AB-Bzpohebq3IaV_G0w11pzGa1L0crGCks5u9o8kgaJpZM4V86Jz .

devotox commented 5 years ago

Released!

devotox / ember-cli-pushjs

Why is firebase a dependency? #26