-
Notifications
You must be signed in to change notification settings - Fork 6
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
RFC: fix CustomClauseQuery for #1836 #1837
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting to this! 🙂
} | ||
|
||
async sourceEnt(_id: ID) { | ||
return null; | ||
return this.source ?? null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this not need to fetch any data? What's the point of passing in the ID if it's always the source from the constructor? My assumption was that it was being reused somehow so that each ent would get this same class...or something? Was this just a legacy API from when it did need to be fetched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to fetch data if used based on id. The source ent is used for privacy checks. In this case, we know what it's visible since you already have an instance to pass to it and don't care, I only used it here for correctness. Can add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. Yeah, if you don't mind adding a comment because I could see coming back to this sometime in the future and wondering if it's a bug.
} else { | ||
this.source = arg2; | ||
source = arg2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this extra variable? Could it just be this.source
that you pass in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, playing it safe for the async function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't resolving. I can try harder to figure out why lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you. There are also some this.source!
uses here too, which gave me a head tilt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't resolve because JS closures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving anyways, not that it matters here. 😛
to address #1836