Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a base model which entity models inherit from #44

Closed
thombruce opened this issue May 23, 2023 · 7 comments · Fixed by #46
Closed

Add a base model which entity models inherit from #44

thombruce opened this issue May 23, 2023 · 7 comments · Fixed by #46
Labels
type: enhancement New feature or request

Comments

@thombruce
Copy link
Owner

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 thombruce added the type: enhancement New feature or request label May 23, 2023
@github-project-automation github-project-automation bot moved this to Todo in Toodles May 23, 2023
@thombruce
Copy link
Owner Author

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: vitejs/vite#9703 There's a workaround suggested here. Don't love implementing "workarounds".

@thombruce
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Owner Author

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.

@github-project-automation github-project-automation bot moved this from Todo to Done in Toodles May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant