A bunch of ideas for improvement

[louisameline]

Hello,
First of all, thanks for your work, the library looks good!
Since you asked for feedback to the community, here is a list of things I'd improve so far, in no particular order. Feel free to ask for details is something is not clear enough.

• The biggest selling point of the library in my opinion is not stated in the docs. For me, it's not so much about avoiding writing my own boilerplate than providing an abstraction for the cloud persistence. With this library, you make no calls to Firestore yourself. You don't have to care about how Firebase works, you just speak to your very own Vuex and the rest gets taken care of behind the scenes. Tomorrow, maybe this library could support other providers and my app wouldn't have to know about it, there would just be another connector to configure to switch to CouchDB, Appolo or whatever
• Also, it's not clearly stated that the library is built upon the concept of optimistic UI, unlike Vuexfire: changes are brought immediately to your store before the xhr calls are made, and you deal with conflicts later if need be (or rather it will hopefully be possible at some point haha), which is the philosophy of the Firestore SDK too.
• I'd emphasize a bit more that your way of doing things is to have a module per collection and/or per document. It became obvious soon, but since I never really felt the need for modules before, I wasn't organized like that, and it required more than 4 lines of code to adjust ;)
• Side note for anyone who uses vuex-persist: I'd recommend to set the minimal size possible (1Mo) in Firestore's persistence options, because otherwise the data is persisted twice (hence less storage space). It should not be completely disabled though because it allows Firebase retries in case of failure.
• I wish dispatch('myModule/delete', '${id}.tags.water') was dispatch('myModule/delete', id, 'tags.water'), using a template in a function argument is cumbersome for no obvious reason
• dispatch('pokemonBox/duplicate', '001') currently returns an object, but it should return a string for consistency with other actions. On the other hand, dispatch('pokemonBox/duplicate', ['001']) should return an array. Makes sense?
• Wording: the vocabulary of the fetch actions could renamed for clarity. I suggest that collections could have a pullCollection() (instead of fetchAndAdd) and a pullDocument(id) (instead of fetchById(id) actions. Document should have just the pullDocument() action. I use pull to diffentiate from a potential fetchCollection for collections, similarly to Git's vocabulary.
• Calling dispatch('myModule/fetchAndAdd') several times in a row for pagination is awkward, something like a .more() method would be nice. I briefly reviewed a library called Fiery a few days ago, it could be an inspiration (or even used by Easy Firestore)
• At some point you say "javaScript object properties do not have an order". This is actually not true anymore for non-integers keys, even though I personnaly still try to avoid relying on the order of Object properties (old habits...).
• A very important rule: don't pollute my state. I know you already plan on not adding the created_at etc. by default in v2 and that's good, but you also shouldn't add the id in the document (or at least keep it local and do not send it to Firestore), and the _sync and _conf should ideally not be in my state. It's as if MySQL stored its config in the user's database. For the id, Vuexfire (and Fiery) keeps the id in the store but sets it as an inenumerable property, which is a nice trick.
• You advise to use a statePropName. I would rather advise not to use one, and keep the data you don't want to synchronize in a "local" module which won't be synced. Also, this feature is redundant with the fact that guards exist to prevent some properties to be synced. If it were me, I'd probably remove this statePropName and add an option that would say "Do not sync properties that start with an underscore", which would be an easy and obvious way to exclude props without the need of guards.
• I'd regroup the EasyFirestore options of a module under a single object to avoid polluting it too much, and make more obvious what's specific to the library
• I see .catch(error => { if (error === 'preventInitialDocInsertion') }) in the doc. Please use new Error('my text') to return an error. You return a string, others will return an object, others an array... The only sensible and consistent thing to do is to use Errors, they exist for that
• You say "It's very important that we take the user to an error page when something went wrong with the Firestore initialisation" but actually you're saying that about the success of enablePersistence, which does not garantee at all that you'll get your initial data right. It's rather upon the failure of openDBChannel or an initial fetch that we need to suspend the app.
• firestoreRefType: 'doc': as a rule, I try to not use diminutives, so I'd use the proper word "document" through the whole app, but that's a personal preference maybe
• statePropName cannot be set to null, you currently have to set an empty string. I've seen this in other places too. This is an unexpected because as a general rule, the absence of a value should be represented by null. Only in very rare cases empty strings should be used.
• An object should be used instead of an array when declaring the modules in VuexEasyFirestore([myModule]). This would remove the need for moduleName in the module. That's the way Vuex works, and in general module don't need to name themselves.
• The library currently requires to declare actions, mutations etc. in an object. However just like in Vuex, it should be possible to provide a module where each mutation is an export, like so:

import * as actions from './actions'
export default {
	firestorePath: 'user/{userId}',
	...
        // this is currently not working
	actions: actions
}

• In Cordova, Firebase cannot be initialized immediately, we have to wait for the deviceReady option. Maybe (I'm not sure about this) we should be able to pass a promise to the library and it would be smart enough to not make the xhr calls and queue them until the promise is fullfilled. But that's a bonus, not a priority

I'll stop for now. I have just started implementing the library actually, I'll probably have other thoughts later if you're interested. Please let me know what you think, so I know if we would be pushing the library in the same direction if I had time to contribute a little.
Thanks !

[mesqueeb]

@louisameline thank you so much for this wonderful list of improvements.
I agree with most of your points!! I'll go over them one by one here:

• The biggest selling point of the library in my opinion is not stated in the docs. For me, it's not so much about avoiding writing my own boilerplate than providing an abstraction for the cloud persistence. With this library, you make no calls to Firestore yourself. You don't have to care about how Firebase works, you just speak to your very own Vuex and the rest gets taken care of behind the scenes. Tomorrow, maybe this library could support other providers and my app wouldn't have to know about it, there would just be another connector to configure to switch to CouchDB, Appolo or whatever
• Also, it's not clearly stated that the library is built upon the concept of optimistic UI, unlike Vuexfire: changes are brought immediately to your store before the xhr calls are made, and you deal with conflicts later if need be (or rather it will hopefully be possible at some point haha), which is the philosophy of the Firestore SDK too.
• I'd emphasize a bit more that your way of doing things is to have a module per collection and/or per document. It became obvious soon, but since I never really felt the need for modules before, I wasn't organized like that, and it required more than 4 lines of code to adjust ;)

You're right on these! I'll improve the docs: #266

• Side note for anyone who uses vuex-persist: I'd recommend to set the minimal size possible (1Mo) in Firestore's persistence options, because otherwise the data is persisted twice (hence less storage space). It should not be completely disabled though because it allows Firebase retries in case of failure.

I don't have any experience with vuex-persist. So feel free to PR the docs for this one ;)

• I wish dispatch('myModule/delete', '${id}.tags.water') was dispatch('myModule/delete', id, 'tags.water'), using a template in a function argument is cumbersome for no obvious reason

Sadly, Vuex actions only allow for 1 single parameter.

And thus my reasoning is this:

• dispatch 'delete' should be usable for both doc and collection modules, while keeping the syntax similar.
• deleting props in a doc is done by: dispatch('docModule/delete', 'nested.prop')
• deleting docs in a collection doc is done by: dispatch('collectionModule/delete', id)
• deleting props of a doc in a collection is therefore done by prepending the id to the sub prop: dispatch('collectionModule/delete', \${id}.nested.prop`)`

I hope you now understand my reasoning. Feel free to continue the discussion on this point ;)

• dispatch('pokemonBox/duplicate', '001') currently returns an array, but it should return a string for consistency with other actions. On the other hand, dispatch('pokemonBox/duplicate', ['001']) should return an array. Makes sense?

I think you are confusing duplicate with another action perhaps. As you can see here, duplicate returns an idMap that maps the new id to the target id that was duplicated.

I return an idMap with duplicate to have the same formatting as duplicateBatch, but you might be right that I could just return the "new id" as string with duplicate and the idMap with duplicateBatch. In version 2, duplicate will be a plugin, and I will make these other improvements at that timing. ;)

• Wording: the vocabulary of the fetch actions could renamed for clarity. I suggest that collections could have a pullCollection() (instead of fetchAndAdd) and a pullDocument(id) (instead of fetchById(id) actions. Document should have just the pullDocument() action. I use pull to diffentiate from a potential fetchCollection for collections, similarly to Git's vocabulary.

We can think of better names for fetch perhaps, but personally I think pull sounds more confusing. 😄 (For me: fetch describes just what it does, it fetches things from the server.)

Perhaps we can rename it to get to make it the same as the Firestore SDK, which is what I'm triggering under the hood.

• Calling dispatch('myModule/fetchAndAdd') several times in a row for pagination is awkward, something like a .more() method would be nice. I briefly reviewed a library called Fiery a few days ago, it could be an inspiration (or even used by Easy Firestore)

Ah, good old pagination... 😃 pagination needs a lot of thinking. Something like .more() sounds nice! But in my opinion this is lowest on my personal priority list, since I don't use pagination in any of my apps, so feel free to take the lead on this one. 😉

• At some point you say "javaScript object properties do not have an order". This is actually not true anymore for non-integers keys, even though I personnaly still try to avoid relying on the order of Object properties (old habits...).

Maybe I should rephrase my statement slightly, but I think this is veery dangerous territory... 😅 Thanks for the heads up though!

• A very important rule: don't pollute my state. I know you already plan on not adding the created_at etc. by default in v2 and that's good, but you also shouldn't add the id in the document (or at least keep it local and do not send it to Firestore), and the _sync and _conf should ideally not be in my state. It's as if MySQL stored its config in the user's database. For the id, Vuexfire (and Fiery) keeps the id in the store but sets it as an inenumerable property, which is a nice trick.

• "but you also shouldn't add the id in the document" → I agree
• _sync and _conf should ideally not be in my state → I agree

As you know, I plan to completely change the syntax to something that's closer to functional programming or the composition api of vue 3, where you'd only import the functions you want to use and avoid polluting anything. I am still thinking about how to best create my library's internal state like _sync and _conf for the next version. So if you have ideas, let me know.

• You advise to use a statePropName. I would rather advise not to use one, and keep the data you don't want to synchronize in a "local" module which won't be synced. Also, this feature is redundant with the fact that guards exist to prevent some properties to be synced. If it were me, I'd probably remove this statePropName and add an option that would say "Do not sync properties that start with an underscore", which would be an easy and obvious way to exclude props without the need of guards.

This one is hard, because it depends on your style. Let me give you an example:

• You would have all docs in a collection sit in the top level of the module, and have other props start with _ or group other props under a single prop called local
• I would rather have all docs in a collection sit inside a prop called data on the module state, then have all my other props I use just sit on the top level of that module's state.

It's just preference of how you would want to do it, and I'm sure I can please more devs by allowing both methods. BUT I do agree it can use some improvement by either making the docs clearer or the improve the syntax of how you pass the config of a v-e-f module.

• I'd regroup the EasyFirestore options of a module under a single object to avoid polluting it too much, and make more obvious what's specific to the library

EasyFirestore options of a module are re-grouped under the _conf state prop. Even though it appears so, they don't stay there sitting on the top level of the module. 😉 But as we discussed above, I wanna get rid of _conf and _sync entirely.

• I see .catch(error => { if (error === 'preventInitialDocInsertion') }) in the doc. Please use new Error('my text') to return an error. You return a string, others will return an object, others an array... The only sensible and consistent thing to do is to use Errors, they exist for that

Not only error handling, but also the fact there's an automatic initial doc insertion on doc mode, is currently a big mess... 😅

• need to rethink how to handle patch actions on docs that don't exist.
• need to rewrite the way all errors are handled in the entire library.... The biggest problem being: not being able to catch errors that come from patches because they get grouped in bulk with a setTimeout...

• You say "It's very important that we take the user to an error page when something went wrong with the Firestore initialisation" but actually you're saying that about the success of enablePersistence, which does not garantee at all that you'll get your initial data right. It's rather upon the failure of openDBChannel or an initial fetch that we need to suspend the app.

You are right. Another doc improvement: #226

• firestoreRefType: 'doc': as a rule, I try to not use diminutives, so I'd use the proper word "document" through the whole app, but that's a personal preference maybe

My reasoning for this was that the Firestore SDK uses doc() and collection() : https://firebase.google.com/docs/firestore/query-data/get-data

• statePropName cannot be set to null, you currently have to set an empty string. I've seen this in other places too. This is an unexpected because as a general rule, the absence of a value should be represented by null. Only in very rare cases empty strings should be used.

yep... good catch 😉 #267

• An object should be used instead of an array when declaring the modules in VuexEasyFirestore([myModule]). This would remove the need for moduleName in the module. That's the way Vuex works, and in general module don't need to name themselves.

aha! clever thought! You're completely right!! But I think I'm thinking of importing actions directly onto the modules for v2, so perhaps a breaking change that might not be worth it anymore for v1. 😢

• The library currently requires to declare actions, mutations etc. in an object. However just like in Vuex, it should be possible to provide a module where each mutation is an export, like so: export default { actions: actions }

WHAT!? 🙉 Very strange... 🧐 I cannot believe why that wouldn't work. It should work lol.

• In Cordova, Firebase cannot be initialized immediately, we have to wait for the deviceReady option. Maybe (I'm not sure about this) we should be able to pass a promise to the library and it would be smart enough to not make the xhr calls and queue them until the promise is fullfilled. But that's a bonus, not a priority

Are you sure...?
I'm initialising Firebase without waiting for deviceReady, and it works fine in all my apps.
The only thing I needed to wait for deviceReady (although I can't remember the reason) was this part:

  function init () {
    Firebase.auth().onAuthStateChanged(user => {
      if (user) return store.dispatch('auth/onSignin', user)
      store.dispatch('auth/onSignout')
    })
  }
  if (window && window.cordova) {
    document.addEventListener('deviceready', init, false)
  } else {
    init()
  }

[mesqueeb]

@louisameline I just wanna conclude to say that I love all your suggestions! Keep them coming while you're working more with the library!!

I look forward on working on V2 with you 😉

[louisameline]

Thanks for the fast answer!
I haven't really looked into what Vue 3 is going to be yet, I'll check it out and hopefully I can help you here and there.

• I wish dispatch('myModule/delete', '${id}.tags.water') was dispatch('myModule/delete', id, 'tags.water'), using a template in a function argument is cumbersome for no obvious reason

Sadly, Vuex actions only allow for 1 single parameter.

I see, so you didn't want an object, fair enough!

• dispatch('pokemonBox/duplicate', '001') currently returns an array

I could just return the "new id" as string with duplicate

I meant "object", not "array", but yes you got my point :)

I don't have any experience with vuex-persist

As the name suggests, it just saves your state locally one way or the other, so you can get it back next time you load the app. Great to save a lot of Firestore calls. I might have more to tell about this in another issue later in case of need ;)

We can think of better names for fetch perhaps, but personally I think pull sounds more confusing.

We actually agree I think, I meant that "fetch" is just a fetch, and "pull" is fetch+add (like pull = fetch+merge in Git). But "get" instead of "pull" is perfectly fine too.

I would rather have all docs in a collection sit inside a prop called data on the module state

Sure, it's a question of preference! I probably wouldn't advertise it as a default way of doing things, but why not.

• I'd regroup the EasyFirestore options of a module under a single object to avoid polluting it too much, and make more obvious what's specific to the library

EasyFirestore options of a module are re-grouped under the _conf state prop

I wasn't clear enough here, I was referring to firestorePath, firestoreRefType, statePropName and other potential properties.

• export default { actions: actions } doesn't work

WHAT!? I cannot believe why that wouldn't work. It should work lol.

Yes, import * as actions actually generates an object of the Module type, and I guess you try to iterate on it expecting it to be a regular object. I had to write actions: Object.assign({}, actions), to make it work.

In Cordova, Firebase cannot be initialized immediately

Are you sure...? The only thing I needed to wait for deviceReady (although I can't remember the reason) was this part

Yes that's what I am referring to. Firebase needs to wait for deviceReady to understand that it's actually running embedded. Trying to use Firebase before deviceReady gets you in a position where onAuthStateChanged is triggered and tells you that the user is not logged in, although he actually is, which leads to errors. In my case, I saw that a call was made to Firestore before my firebaseInit was called, so my options are

1. change my app in order to wait for Firebase initialization before making any editions to the store. Not always straightforward when we're talking about settings initial values into the store that components will use to initialize, but doable with a little time.
2. have v-e-f smartly queue the calls until Firebase is actually initialized. Is there a queue system already built in?

Thanks again!

[louisameline]

I'll close this issue now as everything is going to be addressed in the next version one way or the other