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

Remove Database from Model #98

Merged
merged 2 commits into from
Nov 18, 2021

Conversation

ryandialpad
Copy link
Contributor

@ryandialpad ryandialpad commented Nov 8, 2021

Type of PR:

  • Bugfix
  • Feature
  • Refactor
  • Code style update
  • Build-related changes
  • Test
  • Documentation
  • Other, please describe:

Breaking changes:

  • No
  • Yes

The following attributes have been removed from the Model:

  • _database

The following methods have been removed from the Model:

  • $database
  • $setDatabase
  • $query
  • $delete
  • $deleteByKeyName
  • $deleteByCompositeKeyName

Details

Purpose

The purpose of this PR is to remove Database access from the Model in order to simplify the Model and enforce a repository pattern when using the ORM. This means that we will stop relying on the Model to retrieve another Model and instead use the Repository or Database to fetch any other models.

Changes

  • Removed Database from Model & any other associated methods
  • Removed tests that tie back to the removed logic

import { createStore, fillState, assertState } from 'test/Helpers'
import { Model, Attr, Str } from '@/index'

describe('feature/model/deletes_delete', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following method is deleted so we no longer need these tests.

import { createStore, fillState, assertState } from 'test/Helpers'
import { Model, Attr, Str } from '@/index'

describe('feature/model/deletes_delete_composite_key', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following method is deleted so we no longer need these tests.

Comment on lines -11 to -13
it('throws when accessing the database but it is not injected', () => {
expect(() => new User().$database()).toThrow()
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The database is no longer injected into the model so we no longer need this test.

@@ -22,7 +22,6 @@ describe('unit/repository/Repository', () => {
const user = store.$repo(User).make()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing expectations of injected databases into the Model.

@ryandialpad ryandialpad marked this pull request as ready for review November 9, 2021 17:30
@cuebit cuebit requested a review from kiaking November 11, 2021 00:37
Copy link
Member

@kiaking kiaking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AWESOOOME! Thanks a lot. Let's merge this after we make a new release for MorphOne and withAll and withAllRecursive methods. Since this is a breaking change, I think it's better to push plain new feature release first 👍

@ryandialpad
Copy link
Contributor Author

AWESOOOME! Thanks a lot. Let's merge this after we make a new release for MorphOne and withAll and withAllRecursive methods. Since this is a breaking change, I think it's better to push plain new feature release first 👍

Thanks!!!!! 🙌 🎉

@kiaking kiaking added the enhancement New feature or request label Nov 18, 2021
@kiaking kiaking merged commit a779ac3 into vuex-orm:master Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants