Skip to content

Conversation

cuebit
Copy link
Member

@cuebit cuebit commented Apr 25, 2020

Resolves #93

Type of PR:

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

Breaking changes:

  • No
  • Yes

Details

This PR adds a new optional persistBy?: string option to allow configuring a preferred method of persistence. The default value is 'insertOrUpdate'.

In a nutshell, while this issue comment explains the idea pretty well, the plugin option persistBy can be configured to override the default persistence behaviour.

The signature for this options is: 'create' | 'insert' | 'update' | 'insertOrUpdate'

Beyond that, the plugin will warn users of any invalid value configured and fallback to the default insertOrUpdate method.

N.B. This PR intentionally publicises getDataFromResponse as discussed in a previous issue #112. I think it makes sense to allow users to still be able to retrieve data through configured requests when deferring persistence.

@cuebit cuebit added the enhancement New feature or request label Apr 25, 2020
@cuebit cuebit self-assigned this Apr 25, 2020
@cuebit cuebit requested a review from kiaking April 25, 2020 01:09
@cuebit
Copy link
Member Author

cuebit commented Apr 25, 2020

Drafting this PR pending documentation.

@kiaking your input would be most appreciated.

@codecov
Copy link

codecov bot commented Apr 25, 2020

Codecov Report

Merging #119 into dev will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               dev      #119   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines           80        91   +11     
  Branches        11        13    +2     
=========================================
+ Hits            80        91   +11     
Impacted Files Coverage Δ
src/VuexORMAxios.ts 100.00% <100.00%> (ø)
src/api/Request.ts 100.00% <100.00%> (ø)
src/api/Response.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)
src/mixins/Model.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 95e9db4...ecc64b9. Read the comment docs.

@kiaking
Copy link
Member

kiaking commented Apr 30, 2020

Thank you so much for this! I think it's looking good. Could we have update as an option as well...?

@cuebit
Copy link
Member Author

cuebit commented Apr 30, 2020

Ah yea, forgot to mention, if update is added to the union type, it seems to break the signature compatibility because the update methods signature is more broader than the others.

We could call the method explicitly:

// invoker
this.entities = await this.persist(method, { data })

// invokee
persist(method: PersistOption, payload: any): Promise<Collections> {
  switch (method) {
    case 'create':
      return this.model.create(payload)
    case 'insert':
      return this.model.insert(payload)
    case 'update':
      return this.model.update(payload)
    case 'insertOrUpdate':
      return this.model.insertOrUpdate(payload)
  }
}

What do you think @kiaking?

In the meantime I’ll start adding to docs.

@kiaking
Copy link
Member

kiaking commented Apr 30, 2020

We could call the method explicitly:

Yeah maybe that would be better, since update needs to be supported I think 👍

@cuebit cuebit changed the title feat(response): adds configurable save option feat(response): adds configurable persistBy option May 1, 2020
@cuebit cuebit marked this pull request as ready for review May 1, 2020 04:07
@cuebit
Copy link
Member Author

cuebit commented May 1, 2020

The original idea of configuring the save option meant the save() method would only observe this option if persistence was not deferred.

For example, if a user chooses to defer persistence by setting { save: false }, then later calling save() would withhold the ability to persist through a preferred method.

For that reason alone, reverting back to the original idea of configuring persistBy to a string value seems like the most logical choice.

Users can now defer persistence and save() will still respect the options set by the user:

const response = await User.api().get('...', { save: false, persistBy: 'create' })

// do something

await response.save()

@cuebit cuebit changed the title feat(response): adds configurable persistBy option feat: adds configurable persistBy option May 1, 2020
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.

Ah good point! Nice idea. I like the persistBy option 👍

@cuebit cuebit merged commit 2110528 into dev May 1, 2020
@kiaking kiaking deleted the feature/configurable-persistence branch May 4, 2020 05:59
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