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 relation support #8

Merged
merged 3 commits into from
Mar 30, 2020
Merged

feat: add relation support #8

merged 3 commits into from
Mar 30, 2020

Conversation

cuebit
Copy link
Member

@cuebit cuebit commented Mar 27, 2020

This PR adds relation support.

Since all relations are essentially "sub queries" they do not inherit top level query chain modifiers. This is particularly problematic when using the onlyTrashed modifier, which will never resolve soft deleted relations, because the plugins default behaviour is to ignore soft deleted models and there is currently no informing relation queries to inherit the type of filtering.

Type of PR:

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

Breaking changes:

  • No
  • Yes

@cuebit cuebit added the enhancement New feature or request label Mar 27, 2020
@cuebit cuebit self-assigned this Mar 27, 2020
@codecov-io
Copy link

Codecov Report

Merging #8 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master        #8   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          128       143   +15     
  Branches        13        16    +3     
=========================================
+ Hits           128       143   +15     
Impacted Files Coverage Δ
src/mixins/Query.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 325b720...a8842a4. Read the comment docs.

@kiaking
Copy link
Member

kiaking commented Mar 30, 2020

Very nice! Yea maybe we should be doing something with the Vuex ORM core limitation 🤔 Do you think the PR is ready to be merged?

@cuebit
Copy link
Member Author

cuebit commented Mar 30, 2020

@kiaking yea, I realised when working with TS I’m finding it difficult to tap into certain processes where it would otherwise be straightforward with JS. Currently there is no way for TS to get a hold of relations other than using the query chain, but users may add criteria that may also affect the intended behaviour of the plugin therefore it’s a trivial problem.

Regardless, I’m always keen to find the least expensive solution and this is the best I could come up after hours of alternatives. If the ecosystem is to flourish I think the core api will need to be a little more lenient. But that’s a discussion for another day.

This PR is ready for merge and release. 👍

@kiaking
Copy link
Member

kiaking commented Mar 30, 2020

Yea, you're right. I think we could discuss more on core design 👍

Thanks for this PR! Let's make a release on this 💪

@kiaking kiaking merged commit ed22947 into master Mar 30, 2020
@kiaking kiaking deleted the feature/relation-support branch March 30, 2020 13:06
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.

3 participants