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

Model.$with #45

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Model.$with #45

wants to merge 7 commits into from

Conversation

dan-gamble
Copy link
Contributor

@dan-gamble dan-gamble commented Apr 25, 2023

This is in response to #36

First PR so please bear with me @jplhomer!

This feels like it's been way "simpler" than I was expecting so I feel I'm almost certainly missing something.

The doc wording has been "lifted" from Laravel's docs as I'm really not that great at writing so I felt that was a good place to start. I've not used Laravel but I used Laravel's implementation as inspiration for this.

TL;DR static $with

export class User extends Model {
  static $with = ["posts", "profile"];

  posts?: Post[] | Promise<Post[]>;
  $posts() {
    return this.hasMany(Post);
  }

  profile?: Profile | Promise<Profile>;
  $profile() {
    return this.hasOne(Profile);
  }
}

@dan-gamble dan-gamble changed the title Model.with Model.$with Apr 25, 2023
@dan-gamble dan-gamble marked this pull request as ready for review April 25, 2023 19:01
@dan-gamble
Copy link
Contributor Author

I think this is ready now @jplhomer

Copy link
Owner

@jplhomer jplhomer left a comment

Choose a reason for hiding this comment

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

Awesome work! Thank you. A couple questions:

  • What's your thinking behind $with instead of with? I know the convention is kind of there with the double-relation name thing already, but is it useful to perpetuate to all other sorts of conventions?
  • Does it need to be static? I noticed Laravel's is an instance method instead of a static method.

@dan-gamble
Copy link
Contributor Author

Does it need to be static? I noticed Laravel's is an instance method instead of a static method.

Nope! I swear I was getting a syntax error in my editor when doing this but I don't seem to now. That explains why I went static with it. With that said, because this is at a class level as opposed to an instance level (compared to a relationship accessor) I feel static might make sense still. I'm easy either way.

What's your thinking behind $with instead of with? I know the convention is kind of there with the double-relation name thing already, but is it useful to perpetuate to all other sorts of conventions?

For me I took the $ as evidencing something "magic". I wanted to preserve all static attributes so they can be mapped to the database. A column of with for example would overwrite this or we'd need to provide an explanation in the Proxy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants