feat: support lazy loading services

[pimlie]

Resolves: #231

This PR includes a breaking change, but that could be mitigated by making legacyMode + injectModule default true. Will add docs after first review.

This add 3 new options:

• Bool legacyMode: to inject each service separately, default will now be a single $fire prop
• Bool injectModule: whether to inject the modules/objects or not (works also in legacyMode)
• Bool lazy: whether to use lazy mode

Its not supported to lazy load specific services. Its either all or nothing.

I didnt touch the other plugins, could some help with testing those. Or maybe we can merge them into the service template?

[lupas]

Hey @pimlie

This looks awesome, thanks a lot! Unfortunately won't have time to have a closer look the next 2-3 days - I'll try to review and test it either on Wednesday or on Thursday.

You'll hear from me!

[lupas]

Hey @pimlie

This is awesome. I was meaning to restructure the code-base for a while now and was thinking about splitting the services up, and this is exactly how I imagined it. Adding to that the additional features with the lazy loading, I love it so far.

I started testing all my existing applications with legacyMode, see remarks I found so far in code. Following additional things that came to my mind so far:

• Plugins are renamed, so the movePluginBeforeInitAuthPlugin Helper in lib/helpers/index.js needs to be changed slightly from looking for firebase-module/main to firebase/index on line 8
• In case module fails to initialize, it would be great to have an Info message stating "Version 7 introduced major breaking changes, please read foo.com for how to upgrade or enable legacyMode."
• LegacyMode tests don't cover all plugins yet (e.g. realtimeDb)
• types/index.d.ts would need to be adapted too
• plugins/initAuth.js uses app.$fireAuth.onAuthStateChanged to set the onAuthStateChanged listener. Still works in legacy mode, but stopped working in new mode. Using app.$fire.auth in non-legay mode works, so should be a quick fix.

Regarding legacyMode - what do you think of the following approach?

1. Setting legacyMode per default to true for V7
2. Informing people who have legacyMode === 'undefined` with every start that V7 introduced a new initialization mode and that they should either set legacyMode to true migrate their code.
3. Change it to false with V8

I'll continue testing "non-legacy" mode now (resp. tomorrow), will add some more comments with what I can find.

[lupas]

This PR includes a breaking change, but that could be mitigated by making legacyMode + injectModule default true.

Why do you think "injectModule" default true will mitigate breaking changes? As far as I can see we never injected $fireObj before. What am I missing?

[pimlie]

Thanks for review, have addressed some of your comments. Your proposal for v7/v8 sound good to me, have added a check + info log for this but feel free to adjust the message to your liking.

• When would the module fail to initialize? The idea of the legacyMode is to prevent breaking changes, so not sure what you suggest we'd add (the basic stuff we should add proper tests for)
• have changed the path in movePluginBeforeAuthHelper but I would suggest to deprecate this method. We could use the builder:extendPlugins to provide similar functionality
• plugins/initAuth why is this a separate plugin? cant we move this into the service template?
• The injectModule check was missing for the legacyMode, have added that for legacyMode too

Some other things I noticed:

• We could actually remove all those xxReady vars and just check fire.xx immediately
• Not sure if those writeImport helpers are really beneficial in the service templates
• There might still be an issue with remoteConfig due to the dash in the name

Feel free btw to pair-up with me on this pr if you want and have time, you might be quicker with checking/fixing some stuff then I do as I only have used auth & realtimeDb up to now ;)

[lupas]

Thanks for review, have addressed some of your comments. Your proposal for v7/v8 sound good to me, have added a check + info log for this but feel free to adjust the message to your liking.

Great, perfectly fine like that!

• When would the module fail to initialize? The idea of the legacyMode is to prevent breaking changes, so not sure what you suggest we'd add (the basic stuff we should add proper tests for)

What I meant is: When legacyMode is false by default, users who updated to the new version would instantly face a random error message because they haven't turned legacyMode on yet. But since it's true by default now this has been solved.

• have changed the path in movePluginBeforeAuthHelper but I would suggest to deprecate this method. We could use the builder:extendPlugins to provide similar functionality

Do you mean by letting a user provide an array of plugin-names (in regex) via config which the user wants to be placed after the firebase/index plugin but before the initAuth plugin, and we would then slide these plugins in between? Would that work?

• plugins/initAuth why is this a separate plugin? cant we move this into the service template?

Because we have situations where we want a plugin to be called after firebase init but before initAuth, see for example #146.

Another example, I often use this.$sentry.configureScope within onAuthStateChanged, which would be undefined if the sentry plugin is loaded after the initAuth plugin. So with the helperFunction I can move it in between easily.

• We could actually remove all those xxReady vars and just check fire.xx immediately

Honestly, I never wrote a line of that ready-code so I'm not entirely sure what it is there for :P Totally up for cleaning that up a bit 👍

• Not sure if those writeImport helpers are really beneficial in the service templates

Which one you mean? Would you think it's easier if we hardcode the imports? Problem is that with all the import options we have (static/chunkName/WebpackComments) this would be rather verbose if we don't use the helper in each service template, no?

• There might still be an issue with remoteConfig due to the dash in the name

It has to be imported like import 'firebase/remote-config', where do you see the issue?

Feel free btw to pair-up with me on this pr if you want and have time, you might be quicker with checking/fixing some stuff then I do as I only have used auth & realtimeDb up to now ;)

I'll try to do as much as I can, but have people over for the next 10 days so won't have much time to code, after next week I hopefully can do more :)

[pimlie]

RE: extendPlugins

Yes, exactly like that! That hook returns the full plugins array after resolving, we just have to make sure to re-order in-place. Ie we cannot return a new array but have to use the one the hook receives. Another option would be to copy what nuxt/auth does: https://auth.nuxtjs.org/recipes/extend.html

RE: initAuth

Aha, thx for explaining. So this plugin actually doesnt work atm for lazy mode, because at plugin exec firebase is likely not loaded yet (unless someone adds a plugin before that awaits ready it). If initAuth needs to be run as last, we could just use the extendPlugins hook to always put it as the last plugin. Within the firebase plugin we could then provide an awaitable injection that users can call, they would just need to make sure to never await that injection within their plugin because that would never resolved (ie not await authChanged() but authChanged().then(() =>

RE: xxReady

Yeah sorry, I was commenting on my own code. Progressive insights ;)

RE: template-utils

Yeah, but that extra verbosity could improve code readability, eg if (options.lazy) import is easier to understand then following a function call that has some inner magic. All those templates just arent ideal, but we cant do much about them for nuxt v2 :(

[lupas]

@pimlie

RE: extendPlugins

I'm all in for doing it the way other modules do it, too.

OR we could also just remove it entirely and write a short note in the docs on what needs to be done to move a plugin in between -> that way the user has maximal flexibility and can do it the way he/she wants to.

For example, in my projects I do this:

extendPlugins(plugins) {
      const reorderArray = [
        'firebase-module/main',
        'setupServices', // Private Plugin
        'sentry.server', // Needs to be before initAuth
        'sentry.client', // Needs to be before initAuth
        'initAuth',
      ]
      nuxtPluginMover(plugins, reorderArray)
      return plugins
    },

Where nuxtPluginMover is a helper function (not related to nuxt/firebase) that just reorders plugins the way you input them, but doesn't expect you to input all plugins - just the one's you feed it will be re-ordered.

Could publish that nuxtPluginMover if it is helpful.

That way, in case the user needs to reorder plugins due to some other reason, we don't mess up stuff.

RE: initAuth

Wouldn't all plugins be initialized on page load anyway, so if we lazy load auth e.g. only after a route change the plugin initialization would fail earlier anyway, even though initAuth is set to be the last plugin? I might be misunderstanding something here.

Or is there a way to lazyLoad a plugin in Nuxt? So if the user uses the lazy loaded auth-module, he/she could load the plugin after that?

this.$router.push('/authPage')
// then in the authPage component we load auth and could after that load the initAuth plugin?
this.$nuxt.loadPlugin('initAuth.js') // something like that?

RE: template-utils

You might be right. Making it more readable is imho worth more than having a magic function that strips away every little repetitive string. Go for it!

In general, if you see something that you feel can be cleaned up, feel free to do it. The code grew a lot in the last years without me finding much time to clean it up, and you have some more JS experience than me so I fully trust you here ;)

[pimlie]

Have fixed initAuth, decided to put the subscribe code in the auth server template to prevent having to switch using $fire.auth or $fireAuth. Also I changed the interface a bit (but legacy mode should work the same), you can now subscribe & unsubscribe as many times as you want.

I dont have a strong opinion about all the template stuff because I dont have a need for it atm, if I would have to choose then I would do the same as nuxt/auth v5 does (not sure that that is). But I think we can leave that out of the scope of this pr.

Also other rewrites (ie template-utils) are out of scope for this pr. I mean, it should already work so why change it again. For nuxt v3 we would probably need to refactor anyway.

[lupas]

(deleted comment, issue resolved :))

[lupas]

@pimlie
Thanks for the quick response!

No worries at all, same here, hard to find enough time to finish this up but I think I'll get it done soon :)

I'll make sure to write a proper migration guide and then I think this will be fine without legacy mode.
About the injectModule I will make some test and see how it performs - I guess it'll make sense to split it up.

You wrote further up "We could actually remove all those xxReady vars and just check fire.xx immediately".
What exactly did you mean by that? Do you see a way to automatically load the services the first time they are used without us having to call ready()? (that would be awesome)

[lupas]

Remaining tasks:

• injectModule: Test if it makes difference regarding performance, either inject always everything, or per service individually. If it doesn't make a difference at all, maybe even delete the option to set it true/false?
  -> decided to go with current approach, doesn't seem to make a difference if each module is injected seperately
• stringify issue: Check if it comes from changes in module or if its just me doing something wrong in in the demo, if it's here -> fix it as mentioned above
• tests: Add additional tests for all scenarios and update demo to test every scenario manually

Other than that documentation already looks good to me and it seems to work quite nicely.

[lupas]

@pimlie
I knew what lead to my demo showing "Cannot stringify a function":
It was having this empty fetch() function defined in a vue component:

  fetch() {
    // INFO -> this.$fire.auth user etc. are accessible in fetch
    // INFO -> this.$store.state.authUser is accessible even on server-side in fetch
  },

Removing that got rid of the warning.

Just FYI, I think it's quite interesting. Couldn't reproduce it on a new nuxt repo, so not sure what exactly is the issue here..

[lupas]

@pimlie
Finished the PR and merged into dev, tested everything and it all seemed to work perfectly.

I'll do some more improvements to the module and merge another PR, but then will release all this as v7.0.0 in the coming days.

Thanks a lot for your support and this PR, this is great! =)

[pimlie]

@lupas Thank you so much for finishing this pr! 💯