Refactoring, API Layer, Vuex, README

[roma219]

1. Added an API layer, which handles all the firebase communication (currently only Deliveries)
2. Refactored actions to communicate with that API Layer instead of firebase db directly
3. Refactored Components to use mapGetters and mapActions for getting state data & dispatching actions
4. Changed router to pass params via props and added unknown page redirect
5. Refactored a little according to vue style guide (component names etc) https://vuejs.org/v2/style-guide/
6. Added info on style-guides (eslint & vue) to README
7. Removed outdated info from README
8. Separated connected components by folders
9. Refactored mutations so they are in CAPITALS
10. Added mutations for adding/saving/removing deliverie
11. Converted deliveries from Array to Object for easier access (remove, edit)
12. Added some pending buttons state to Delivery Form

[nicksarafa]

we should probably get a more robust gitignore so every addition to the package.json doesn't result in 15000+ changes

[roma219]

@nicksarafa Right, i'll add package-lock.json to .gitignore

[nicksarafa]

yeah list *.lock files inside the gitignore is considered a bad practice but it makes the pull requests annoying and unreadable. i propose we..

1. add *.*lock to the .gitignore as well as pull
2. leverage a standard node .gitignore template

[piggydoughnut]

you can just collapse the .lock file when reading the PR. Don't add it to .gitignore, everyone needs to work with the same versions of stuff.

[piggydoughnut]

also we decided to use yarn if I am not mistaken, so make the yarn.lock file instead of package-lock.json

[nicksarafa]

@piggydoughnut yeah we can do either or. I just prefer smaller pulls with an accurate count of changes in the log rather than 15,000 for every pull request pushed up

[JollyBrackets]

@roma219 lot of cool stuff there, thanks! But I have some questions/comments:

API Layer
Is this necessary? This makes Vuex even more verbose and we now have 2 files per entity/collection.
Especially for a firebase project (which could handle the state itself) this just seems overkill.
I really like what you did with aysnc/await though. @mediashane Is this what you wanted to do?

Routing Props
Seems like a good idea. I also added vuex-router-sync in this branch.
With this we could handle all the state (including routing & params) through vuex.
Which do you like better?

Moment.js
I replaced the vue.prototype.$moment with the vue-moment plugin, which adds filters as well.
Is moment.js that slow? I really like it for its simplicity and it now also supports locallisation.

Changes from latest PR
Today I created this PR which also contains quiet a bit of refactoring (due to the separate main routes).
There will be quiet a few conflicts like the folder structure, routing, README.
Let me know if you need help resolving.

[roma219]

@JollyBrackets
API Layer
Well, what it does is it delegates all the external API communications (firebase in our case) to a module with simple methods to perform them, which actually makes Vuex less verbose - this way actions do not know the source of data, urls and other api communication specificities, they only use this API module methods to get data. i think it makes actions more clean.

Compare:

fetchDailyList: async ({ commit }, { date }) => {
    const result = await API.Deliveries.fetchDailyList({ date })
    if (result.success) {
      commit(types.FETCH_DAILY_DELIVERIES, { list: result.data })
    }
    return result
 }

and

fetchDailyList: async ({ commit }, { date }) => {
  try {
    const response = await db.collection('delivery')
      .where('timestamp', '>=', new Date(date))
      .where('timestamp', '<=', new Date(date.endOf('day')))
       .get()
    const data = []
    response.forEach(doc => data.push({ id: doc.id, ...doc.data() }))
    commit(types.FETCH_DAILY_DELIVERIES, { list: result.data })
    return { success: true, data }
  } catch (error) {
    return { success: false, error }
  }
}

If there is not gonna be a big number of firebase requests, we can use just a single api.js file for storing methods, don't need to specifically separate them into several files (like api/deliveries.js etc). Not sure what you mean by saying that "firebase can handle the state itself" - you mean actual vuex state or? For me right now seems from a vuex perfective it is just an endpoints to make regular asynchronous requests to. What do you think?

vuex-router-sync
Seems unnecessary to me to use is just to deliver router data to component throught vuex state (when you can use it through $route object or props). As far as i understand, it is needed only when you want to use router object inside vuex, so if you got an idea for that - why not, but otherwise i don't see a need to use it.

Moment.js
I didn't mean to say moment.js is bad or slow - it is a pretty great library, but there is a better alternative - take a look at https://date-fns.org/ - kinda the same, but it performs better and is lighter + it has an ability to import only modules you need, not the whole package (like moment which is pretty big) or nothing. Well, i just looked over vue-moment - looks handy for this kind of weekend-project, should stick with it for now, maybe switch to date-fns later.

Changes from latest PR
Yeah, i saw it, i'll merge it to my branch tomorrow morning - i think i'll be able to resolve the conflics, i'll contact you if i need help.

[mediashane]

@JollyBrackets Yes sir that is what I was thinking of doing with async/await. Makes it a little cleaner and helps avoid callback hell :) I dig the API layer too. Do we just need to make a decision about Moment.js vs date-fns before approving the merge? Or is @roma219 still working to resolve conflicts from yesterdays PR?

[JollyBrackets]

@roma219 cool, thanks for the answers. Here some more comments :)

API Layer
I see what you mean and generally you are right and this is a clean way to do it for big modular apps.
However I think it's overkill since we are building a CRUD app with firebase.
Regarding verbosity: yes vuex itself get's less verbose, but overall we have more code per action.
Regarding firebase state: Since it's realtime capable you could just use the firebase object itself to render the components and use its built-in actions.
So even building a vuex-store on top of firebase is a bit unnecessary but then having an API layer is just overkill imo.

Vuex-Router-Sync
Yeah you might be right, I just it might be cool to have everything in the store.

Moment
A cool, I didn't know about date-fns. But if it's faster and lighter definitelly go for it!

[roma219]

@JollyBrackets
I've updated this PR by merging extended-setup branch, so currently waiting for extended-setup branch to be merged into develop.

API Layer
Well, you might be right about it being a little overkill, i have mixed feelings about it now, but even now from my perspective it just looks cleaner this way... I mean, it is just basically one more file and yes, a little bit more code, but it just gets more structured this way. Who knows what awaits our little project in the future, maybe it will grow? :) It just doesn't seem like this approach has any big drawbacks for me...
We probably could have done everything without vuex and api layers, just in-component fetching, displaying and saving data, but wouldn't it make components some huge monsters with too many lines?

Still don't understand how can firebase data object "render components" reactively, you still need to make requests through it, don't you?

Maybe we can settle with some other vuex modules accessing firebase directly and switch to API Layer structure later on if we feel like it's needed (or remove api layer completely if it feels non-needed later on)?

[mediashane]

@roma219 If I'm understanding @JollyBrackets correctly, every time one of the table components renders it fetches the relevant Firebase collection. So in practice Firebase is acting as a sort of 'remote' state store, so local state doesn't need to be passed as props through the router, or even kept in a vuex-store for that matter.

We should probably even add a Firebase event listener for any database value changes, and then push any new values to the component so it updates the tables any time there is a change or addition: https://firebase.google.com/docs/database/admin/retrieve-data#section-event-types

As for the API layer, if we can imagine this project ever needing one down the road, we might as well add it now, right? Otherwise as the project grows, it'll become more and more of a pain implementing it.

[roma219]

@mediashane "every time one of the table components renders it fetches the relevant Firebase collection" - the question is, is this somehow automated and reactively changed, or you still basically make request in created() component callback to fetch the data and then store in in component's data to display it? Because this way it doesn't seem any different from any other restful api (which is how it is right now)

[mediashane]

@roma219 Yeah if I'm understanding everything correctly it is fundamentally RESTful. The request is made in created(), the response from Firebase is dispatched to the component's state store, and then the component updates to display that new state. We could automate this process by implementing a Firebase event listener like the one I linked above, so that the app's tables are updated in real time. Otherwise, as things are now, a user would have to make a navigation action in order to 'refresh' the contents of a screen (though admittedly this only really becomes an issue when we have more than one Banjar/Facility Manager using the app).

[roma219]

@mediashane Well, this is how it is done now, but with a Vuex you separate view layer (vue components) and data logic (vuex store), so the requests are done in vuex actions, which update the state with a new data, and components just display it throught state getters. I don't see a problem with a potential implementaion of this firebase listeners, who would just call some vuex actions, who in response will fetch the new data etc. It just about the separation of concerns.

[roma219]

@JollyBrackets @nicksarafa fixed the conflics & travis build

[roma219]

@nicksarafa merge pls