-
Notifications
You must be signed in to change notification settings - Fork 4
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
chore: move entity loader utils into their own object #239
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #239 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 71 72 +1
Lines 1934 1949 +15
Branches 267 267
=========================================
+ Hits 1934 1949 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
From an API standpoint what makes you not want these methods on the top-level loader? When I think about the desired end state, loaders would be enforcing by default and most code would look like XEntity.loader(vc, qc).operation()
. XEntity.loader(vc, qc).nonEnforcing().operation()
would be the exception.
In terms of types, XEntity.loader(vc, qc)
would conceptually return EnforcingLoader | Utils
and XEntity.loader(vc, qc).nonEnforcing()
would return just NonenforcingLoader
.
I prefer Most of the calls in www are of form |
I think the reason I prefer them not be top-level is that they are so rarely used compared to loader methods, and shouldn't be used by normal applications except in exceptional cases. Some data from Expo backend:
So to me the API that makes the most sense in the end (for now, we can't put enforcing loader methods on top-level yet due to implicit breaking change) is:
|
ea0bea9
to
9d546c6
Compare
This is a good point. "utils" in general is a pretty poor term. But in this case it's a bunch of utility methods that are both used by the loaders themselves as well as (super rarely) used by the application. They don't have a "enforcing" or "withAuthorizationResult" property (the methods in utils don't use results or throw errors or load entities), so in this case I think they belong elsewhere. And "utils" is all I got as far as creativity lol.
I think theres a slight misunderstanding in the set of methods being put in utils. It's just things like So after this PR it is:
|
Why
This is the second part of #238. From the summary:
This PR is this follow-up. It creates a
utils
concept that the loaders use but also that end-users can use in rare cases (we use them in rare cases in Expo server code).The reason for this is that it doesn't make sense to have a call of the form (what they are after #238):
AssetEntity.loader(viewerContext, queryContext).nonEnforcing().invalidateEntityAsync(asset)
Instead, this PR makes it:
AssetEntity.loader(viewerContext, queryContext).utils().invalidateEntityAsync(asset)
Before both PRs, it was:
AssetEntity.loader(viewerContext, queryContext).invalidateEntityAsync(asset)
But I think it's better to not have them on the top-level loader for cleanliness.
Test Plan
Run all tests.