thombruce / toodles

✅ A super simple todo app
https://toodles.thombruce.com/
GNU General Public License v3.0
0 stars 0 forks source link

Add a base model which entity models inherit from #44

Closed thombruce closed 1 year ago

thombruce commented 1 year ago

As is, Interval, Tally, Comment and even Todo models share a bunch of common setup. And IntervalCollection, TallyCollection, CommentCollection and TodoCollection are practically identical (though if they are reduced to a common model, it should accept a parameter for uniqueness and indices setup on individual types).

Reduce code repetition by setting up a base model which the above models inherit from. Extend the base model on specific models with only what is unique to them, significantly reducing repetition and improving readability of individual classes.

thombruce commented 1 year ago

This is gonna be a little tougher than I thought:

TypeError: Class extends value undefined is not a constructor or null

I don't think the code I've written is incorrect:

import { BaseCollection } from './BaseCollection'

class TodoCollection extends BaseCollection {
  // Constructor
  constructor() {
    super('todos')
  }
}

export { TodoCollection }

Could be related to: https://github.com/vitejs/vite/issues/9703 There's a workaround suggested here. Don't love implementing "workarounds".

thombruce commented 1 year ago

Likely a case of circular dependencies...

  1. BaseCollection imports db
  2. IntervalCollection imports BaseCollection
  3. useIntervalsStore imports IntervalCollection
  4. db imports useIntervalsStore

BaseCollection needs to import db. The point of it is to initialise the database. So... I guess we have to figure out how to finally remove those nasty store dependencies from the database plugin.

thombruce commented 1 year ago

Dependencies were already circular before this. I don't know why now that I've made one simple change it's a problem all of a sudden. 🤷‍♂️

thombruce commented 1 year ago

I've managed to fix the issue outside of the testing suite, which is promising. So now if I can just fix whatever's wrong with the tests...

thombruce commented 1 year ago

A while back, I discussed the desire to move the initStore functions to the components that require them.

I've just added the basic functionality that should allow this. A useGlobalsStore() that the Loki database plugin calls after init; it sets a ready state property to true. In App.vue, we check this property to determine whether to show a loading text or the main RouterView component.

Now if I can just move all of the initStore() functions to views/components that require them, I should be good...

In fact, this is what managed to restore functionality to the main app. The test suite complains because I'm still initialising stores manually there... the workaround for which is what?

The stores do need to be initialised in order to test their behaviours. The stores do require the db import... and the db is no longer importing them directly? But I am importing them in the test files... and... what? I'm struggling to see where the circular dependency is now.

thombruce commented 1 year ago

Take ActiveInterval.spec.ts for example:

  beforeEach(() => {
    // TODO: We should probably createTestingPinia and setup mocks of internal actions
    setActivePinia(createPinia())
    useIntervalsStore().initStore()
    useTodosStore().initStore()
  })

It's initialising both the intervals store and the todo store. These each call their respective collection (IntervalCollection and TodoCollection) which each import the Loki database as db. The database plugin now only imports the globals store, but this globals store calls each individual store in order to init it. This completes the circle.

So if we remove those store references from useGlobalsStore (and, as planned, put the initialisers in views and components instead), this should resolve the issue... No?

Worth a test. Quickest way to check is to put the init logic in... probably not App.vue, but the top level RouterViews for now. We'll sort them into components as needed later.

thombruce commented 1 year ago

That works!! Will create a PR soon... _clearly gonna have a lot more scope than I would have liked, but I'll try to organise the commits into sensible, smaller steps.