Closed koskimas closed 2 years ago
@koskimas Thanks for the explanation, I think if we were to do something it would be about making relate and unrelate similar in usage.
or use whatever SQL query you want. You wouldn't be able to do that if we changed the API to take an id.
I must be missing some challenges about why relate(id) is ok but unrelate(id) is not ok (besides the current implementation that does not accept it)? Can't we express in current code "If unrelate is called with a parameter, then it's a 'unrelateById' call"?
Whatever we make, if relate can be called relate(id) and unrelate can't then it will always come as a suprise to a developer building API routes like:
I re-read your answer and understands the current unrelate() goes beyond just removing user from a team (thanks to where etc..) but I am not sure to understand why we could not have the best of both worlds :D
Because having two ways to do one thing is confusing. As I said, I can add unrelateById
method that would work the way you want. You don't seem to complain about how delete
works? The exact same arguments on both sides apply to the delete
method.
@koskimas
await user.$relatedQuery('teams').unrelate() await user.$relatedQuery('teams').relate(newIds)
Yeah, this does the job. However I'd expect this -sync
method selects all relations, grabs their ids, matches with provided ids and accordingly runs insert ids, delete ids.
I believe upsertGraph
example could also work, but I was so turned away after seeing a huge warning in docs about using it only it if you can't use simple methods because of complexity, problems, etc. After reading this I quickly switched to relate
, unrelate
and was also surprised to see an error after using unrelate(ids)
.
Then I had this case where I needed to compare existing and provided ids and relate/unrelate accordingly. I quickly checked if there is an easy way to do this, didn't find one except upsertGraph
with scary warning and checked how it's done in laravel docs (even after years I'm amazed by its architecture). Found there methods:
$user->roles()->attach($roleId);
$user->roles()->detach($roleId);
$user->roles()->sync([1, 2, 3]);
translate to objection
user.$relatedQuery('roles').relate(roleId)
user.$relatedQuery('roles').unrelate(roleId)
user.$relatedQuery('roles').sync([1, 2, 3])
Looks so natural and simple to me, wondering on your thoughts about actually having this.
Because having two ways to do one thing is confusing.
Definitely agree on this for APIs, always confusing: which way to choose
But I am convinced that if you have an API method like deleteFile(path) and you have undeleteFile().where("path", path) then the developer experience is not usual. The natural way someone would try to use undeleteFile() would be undeleteFile(path), it seems obvious from the user perspective. If the method are named the same with just the addition of "un" then people will try what I tried. The symmetry of the API here seems important given the names relate/unrelate.
If unrelate was not an API method I would not even complain: I am fine using another way, but since the method is here, I tried to use it the same way I used relate().
I will stop here trying to push for this and say: I always enjoy working with objection.js and wanted to provide you this feedback about unrelate.
About delete: I did not encounter a case where I was surprised by the API but if I ever encounter such case I will make sure to always give you the feedback!
Good luck! And thanks for the library again.
I think the main thing that is getting lost here is that objection is a query-builder, so the semantics of the API is fairly one-to-one with the queries that it is generating (unlike Eloquent's ActiveRecord interface, for example). Just as the semantics of SQL's INSERT
and DELETE
are not symmetrical, objection's relate()
and unrelate()
are also not symmetrical.
I'm hiding these comments to keep this thread readable. Let's move further discussion elsewhere. Feel free to open separate issues. @vvo @vladshcherbin
Not a specific request, but after using Objection for a couple months I feel like the API offers too many ways to do the same thing. For example, I'm currently trying to figure out the difference between fetchGraph
and withGraphFetched
, and why would I choose one over the other.
If Objection intentionally provides several different approaches to ORM, it would be nice to make this explicit. Otherwise I'd very much like to see as many methods as possible deprecated.
@Kinrany I disagree. The difference between these two is pretty clear.
fetchGraph
is used on an instance of a model. So you'd use it when you have fetched something from a table, but for some reason you need to fetch additional data via relations.withGraphFetched
is on QueryBuilder. You use that if you know upfront that you will need data from those relations. Maybe this is more on knex level but I would love to see something like the Sandbox Adapter from ecto (https://hexdocs.pm/ecto_sql/Ecto.Adapters.SQL.Sandbox.html).
This allows for pretty painless and fast integration tests.
q.where(ref('data:prop.list').arrayLength())
@koskimas I would like to see a section which run micro query directly from query param of API calls. I have created a patter for micro query -> then I need to convert this patter to raw query and finally use objection object .whereRaw() to execute the same.
Change the api for $fetchGraph. At the moment it accepts an option {skipFetched: true}
.
I would very much prefer if it instead by default behaved like when {skipFetched: true}
and to force refetch of related items, you would use a different method-could be something like
$fetchGraphForce('user')
To me seeing a code with a different method name is more explicit than the opt object.
IMHO in general usecase where you use objection on an API, you typically don't need to always refetch relations.
Change the api for $fetchGraph. At the moment it accepts an option
{skipFetched: true}
. I would very much prefer if it instead by default behaved like when{skipFetched: true}
and to force refetch of related items, you would use a different method-could be something like$fetchGraphForce('user')
To me seeing a code with a different method name is more explicit than the opt object. IMHO in general usecase where you use objection on an API, you typically don't need to always refetch.
Thank you… will surely look into this… 😊
I'm starting to plan the 2.0 release and I'd like to hear the community's opinions what we should add to objection in 2.0 or later down the road.
I'd like to reiterate here that the goal of objection was never to become Eloquent, Django's ORM, Active Record or any other known ORM. Objection's goal is said pretty well at the top of the github README. Objection is a "query builder on steroids" that never get's in the way of SQL, but still attempts to provide tools for working with repetitive stuff and especially relations.
That said, there are many things those ORMs do better than objection and we should try to bring in the features that suit objection's design goal.
SO, I'd like you to post here what you miss from any other tool you've worked with and think objection should really have. Vote the features using :+1: and :-1:
Couple of things 2.0 will have:
eager
is used as a verb, even though it's not,joinRelation
implies it joins a single relation.joinEager
sounds likejoinRelation
even though it does a very different thing. These will be something likewithGraphFetched
,joinRelated
,withGraphJoined
. etc. The old names will remain as aliases at least until 3.0.https://github.com/Vincit/objection.js/projects/1