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

feat: add save and revive method #62

Merged
merged 5 commits into from May 17, 2021
Merged

feat: add save and revive method #62

merged 5 commits into from May 17, 2021

Conversation

kiaking
Copy link
Member

@kiaking kiaking commented Apr 27, 2021

ref #60

This PR adds 2 new method to the repository. save and revive.

Type of PR:

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

Breaking changes:

  • No
  • Yes

Details

The save method will insert a given data, or update it if it already exists in the store. It will also normalize any relationships as well.

userRepo.save({ id: 1, name: 'John Doe' })

The save method will return something we call schema, which is the original data passed into the save method, but with special __id key attached.

const data = {
  id: 1,
  posts: [
    { id: 2, userId: 1 },
    { id: 1, userId: 1 }
  ]
}

const schema = userRepo.save(data)
/*
  {
    __id: '1', // <- Attached!
    id: 1,
    posts: [
      { __id: '2', id: 2, userId: 1 }, // <- Attached!
      { __id: '1', id: 1, userId: 1 }  // <- Attached!
    ]
  }
*/

We may now pass this schema to the new revive method to retrieve the models.

const user = userRepo.revive(schema)
/*
  User {
    id: 1,
    posts: [
      Post { id: 2, userId: 1 },
      Post { id: 1, userId: 1 }
    ]
  }
*/

All of the relationship will be correctly resolved. Also, note that the order of the posts relationships are well preserved.

Combining save and revive method, we can easily reuse the record order that external apis might had, while normalizing the data and saving it to the store.

Todos

  • Resolve TODO comments

@kiaking kiaking added the enhancement New feature or request label Apr 27, 2021
@kiaking kiaking self-assigned this Apr 27, 2021
@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #62 (ea237e5) into master (9a93d0f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #62   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           36        36           
  Lines          838       901   +63     
  Branches       134       144   +10     
=========================================
+ Hits           838       901   +63     
Impacted Files Coverage Δ
src/connection/Connection.ts 100.00% <100.00%> (ø)
src/interpreter/Interpreter.ts 100.00% <100.00%> (ø)
src/model/Model.ts 100.00% <100.00%> (ø)
src/modules/Mutations.ts 100.00% <100.00%> (ø)
src/query/Query.ts 100.00% <100.00%> (ø)
src/repository/Repository.ts 100.00% <100.00%> (ø)
src/schema/Schema.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a93d0f...ea237e5. Read the comment docs.

@ryo-gk ryo-gk mentioned this pull request Apr 28, 2021
10 tasks
@cuebit
Copy link
Member

cuebit commented May 7, 2021

The only obstacle here is the use of Vue.set. While this works for Vue 2, it will not work for Vue 3. The import statement is technically compatible with Vue 2/3 but will break with Vite.

Investigating this further, the following is the best to my knowledge:

  1. Custom set method (see below)
    Pros: require('vue') works with Vue 2/3 + Webpack and Vue 3 + Vite.
    Cons: does not work for CDN versions. I lied, it works.
  2. vue-demi as a peer dependency
    Pros: provides universal treatment.
    Cons: requires VCA as a peer dependency itself which seems like a rather large overhead (for users not using VCA) just for the use of set.
  3. Wait for Vue 2.7
    Pros: Doesn't require intermediary dependencies.
    Cons: Does require Vuex ORM to fix 2.7 as a peer dependency.

Custom set method bridges Vue 2 & 3 (inspired by vue-demi and vue-use, except it does not require a post-install fix):

let Vue: any
try {
  const m = require('vue')
  // Webpack requires `.default`, Vite does not.
  Vue = m.default || m
} catch (e) {
  Vue = window.Vue
}

export function set<T>(array: T[], key: number, value: T): T
export function set<T>(object: Record<string, any>, key: string | number, value: T): T
export function set(target: any, key: any, value: any): any {
  // Array does not require treatment, the following maintains reactivity in both versions.
  if (isArray(target)) {
    target.length = Math.max(target.length, key)
    target.splice(key, 1, value)

    return value
  }

  // Object does require treatment.
  // Vue 2 requires `Vue.set` to maintain reactivity.
  // Vue 3 simply reflects the object values.

  assert(Vue && typeof Vue.version === 'string', [
    'Vue was not detected. Please install Vue.'
  ])

  const version = Vue.version.slice(0, 2)

  assert(['2.', '3.'].includes(version), [
    `Vue version ${Vue.version} is not supported.`
  ])

  if (version === '2.') {
    return Vue.set(target, key, value)
  }

  target[key] = value

  return value
}

Screenshot 2021-05-08 at 00 27 23

Screenshot 2021-05-08 at 00 27 25

@cuebit
Copy link
Member

cuebit commented May 8, 2021

Thinking about this more, it's becoming more evident the libraries migrating to Vue 3 are adding VCA as peer dependencies for Vue 2 support. So I think this might be the better option.

@kiaking
Copy link
Member Author

kiaking commented May 10, 2021

Oh, didn't know vue-demi offers set 👀 Hmmm... it might be good choice. Not sure. Depends when I want to merge this one, but maybe we could start off by not using Vue set, then rethink about it together with the performance measurement.

@kiaking kiaking merged commit d45825e into master May 17, 2021
@kiaking kiaking deleted the save-revive branch May 17, 2021 05:55
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.

None yet

3 participants