-
Notifications
You must be signed in to change notification settings - Fork 5
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
Version 2 RFC #18
Comments
This looks great! 🚀 😄 Some quick thoughts:
app.hooks([
async context => {
const {
id,
method,
path,
// Pull loader and remaining params
params: { loader, ...params },
} = context;
if (loader && method === 'get') {
context.result = await loader(path).get(id, params);
}
if (loader && method === 'find') {
context.result = await loader(path).find(params);
}
}
}); |
Thanks for the response! There is quite a bit of context and history on why I chose the
const singleLoader = DataLoader.loaderFactory(
app.service("service"),
"id",
false // false === '!' === required sing object
);
const multiLoader = DataLoader.loaderFactory(
app.service("service"),
"id",
true // true === [!] === array of required fields
);
// We have four methods to fetch data
// singleLoaders are generally used when this record holds the foreignKey and
// generally map to the `get()` method.
singleLoader.load(post.user_id); // returns 1 required object
singleLoader.loadMany(post.user_ids); // returns array of required objects, **one object per id
// multi loaders are mostly used when the other record holds the foreignKey and
// are analogous to the `find()` method...sorta, its a find with a keyed relationship
multiLoader.load(post.id); // returns an array of objects matching this id
multiLoader.loadMany(post.categories); // returns and array of arrays, **one array per id So there was some confusion which methods to use and it was also tedious to setup loaders for each scenario. I too thought about some way to map these methods and make sense of get, find, load, loadMany, loadManyMulti... // load takes an id or an array of ids. It replaces both methods
// from the singleLoader
loader.load(id) // return one record
loader.load([...ids]) // return an array, one record per id
// loadMany takes an id or an array of ids.
// It replaces both methods form the multiLoader
loader.loadMany(id); // returns an array of records
loader.loadMany([...ids]); // return an array of arrays, one array per id; That means that Heres a more realistic example // Common, straightforward load a "parent". Probably most used
userLoader.load({ id: post.user_id });
// Less common, but definitely still used, particularly
// with document databases
categoryLoader.load({ id: post.category_ids });
// Common, load many "children"
starsLoader.loadMany({ post_id: post.id });
// This one is weird...can't really think of a use case. Its generally
// not an id relationship but some other single property
topicsLoader.loadMany({ topic_code: post.topics }); So with the id object things, get/load returning an array, loadMany/find needing a keyed relationship...I thought it would be more confusing to try to map them to get/find and let them be totally separate. Its also important to note that there is a 1-1 map for both I would like to also consider actual relationship names, but I had trouble reasoning about it really. It confused me. Maybe someone has some ideas // userLoader.load({ id: post.user_id });
userLoader.belongsTo({ id: post.user_id });
// or is it the other way around?
userLoader.hasOne({ id: post.user_id });
// I think this has potential. But its not quite right...
// the relationships used do not truly map to these either
userLoader.oneToOne();
userLoader.oneToMany();
userLoader.manyToMany();
userLoader.manyToOne(); So that where we are with names...I landed on I like the suggestion of passing a "whitelist" array to the methods as third argument. That can clean some things up. I will think on that for sure. |
You can also now checkout the code on the v2 branch |
I would love some feedback from @marshallswain , @DesignByOnyx, @mrfrase3, @KidkArolis, @fratzinger, @robbyphillips. And of course anyone else! Just pinging ya'll to bring some awareness to this post. |
Unfortunately, I don't have much to add as I do population on the client-side. But keep up the good work! |
@mrfrase3 This totally works client side too 👍. |
I have created an example app showing how all of this works.
|
After some further consideration and further use in my own production apps. I have really come around on @daffl suggestion of removing the the context.params.loader('comments').load(post.id, null, { loader: context.params.loader });
// and/or
context.params.loader('comments').load(post.id, null, { transaction: context.params.transaction }); Note that we have to pass context.params.loader('comments').load(post.id, { query: {...} }, { loader: context.params.loader }); But I just hate having to pass Instead, I am going to update the code such that it takes some config that is a "whitelist" of params to stringify into the cache key. This would probably be set to // the stringified cache key is just the post.id
context.params.loader('comments').load(post.id, { loader: context.params.loader });
// the stringified cache key is the post.id and query
context.params.loader('comments').load(post.id, {
query: { ...the query },
loader: context.params.loader
});
// can also pass in a whitelist as third param to override default
context.params.loader('comments').load(post.id, {
query: { ...the query },
myParam: { ...my custom param }
loader: context.params.loader
},
['query', 'myParam']
); I will like update the const usersLoader = new ServiceLoader(app.service('users'), {
whitelist: ['query', 'myParam'],
...other dataloader config like maxBatchSize, etc
})
const lazyLoader = new LazyLoader({
users: {
whitelist: ['query', 'myParam'],
...other dataloader config like maxBatchSize, etc
},
comments: {
whitelist: ['query', 'myParam', 'anotherParam'],
...other dataloader config like maxBatchSize, etc
}
}) |
Another random thought, which I think is quite exciting. I have been thinking of this mainly as a loader is created at the beginning of a service call (probably via app:before:all hook) and then manually passed to all "nested" service calls. That loader lives and dies in the course of that single request and that is an important aspect because it makes a loader different than a cache (which has to be invalidated). That is the most natural way to think of this and what most of my examples and tests reflect. But, there is no reason that multiple service call's can't use the same loader. For example, const lazyLoader = new LazyLoader();
const loader = lazyLoader.loader;
await Promise.all([
app.service('users').get(1, { loader }),
app.service('posts').find({ query: { published: false }, loader }),
app.service('posts').find({ query: { published: true }, loader }),
]); There are times when a developer knows that multiple "requests" can share a loader and can manually create one and pass it to all of them. This is going to offer even more performance benefit. This would also pair well with |
This looks incredible! I use current batch loader all over the place in our production app (thank you very much for it, along with |
@1valdis The underlying Currently, the Considering your comment about the upgrade path I will not remove |
@DaddyWarbucks as for our usage, we have abstracted batch loader creation into a factory, because, as you've mentioned, it requires quite a few settings to create (I don't say it's a bad thing! I'd rather have something I can easily customize instead of something so dumb it can do only a simple job). Then, we currently have 58 batch loader factories on top of that one, most of which are just one factory per service, some however doing things like count+groupBy. Each request is wrapped in a transaction so all calls pass it around as well. In total I found 267 calls to We use We also use additional {
[key]: { $in: getUniqueKeys(keys) },
...query,
}
As for the questions asked in the starting post.
Hopefully that helps :) |
I have updated this as per some of our discussions in this thread.
// The following would only keep `query`
loader('posts').load(1, { query: { published: true }, myProp: true } });
// The following would keep `query` and `myProp`
loader('posts').load(1, { query: { published: true }, myProp: true } }, ['query', 'myProp']);
// The following would aslo keep `query` and `myProp`
loader('posts').load(1, { query: { published: true }, myProp: true } }, params => {
return { query: params.query, myProp: query.myProp }
});
const contextLoader = new ContextLoader(context);
contextLoader.service('users').load(1)
const contextLoader = new ContextLoader(context);
contextLoader.service('users').load(1);
contextLoader.service('users').loadMulti(1);
contextLoader.service('users').get(1);
contextLoader.service('users').find();
contextLoader.service('users').clear(); // clears all loader types
contextLoader.service('users').clear(1); // clears all loader types with id = 1
contextLoader.service('users').clear(1, { ...params }); // clears all loader types with id = 1 AND matching params There were a couple of other little things that changed under the hood. But the most important piece of this new rewrite is the class ServiceLoader {
load(idObj, params, filterParams) {
// Load 1 item per id in idObj
// Basically a hasOne relationship
}
loadMulti(idObj, params, filterParams) {
// Load N items per id in idObj
// Basically a hasMany relationship
}
get(id, params, filterParams) {
// Get cached item at id/params
}
find(params, filterParams) {
// Find cached items at params
}
clear(idObj, params, filterParams) {
// If no idObj or params provided, clear all caches
// If no idObj but params provided, clear all caches matching params
// Else if idObj and no params, clear all ids and all finds
// If both passed, clear all matching id/params and all finds
}
} I need to add a bunch of tests and types. But I think this is generally complete. I am using a forked version of this in a production application that is also using |
Sounds neat, I'd greatly appreciate the code example 👀 |
@1valdis See: feathersjs/feathers#2074 (comment) for a quick example of this paired with |
Hey all!
This library has moved to feathers-ecosystem. Along with that move, I would like to make some updates and improvements. The library is amazing, but hasn't received the adoption I believe it deserves. I have used it in many production apps with a few different patterns and have landed on what I believe to be some nice additions to the library.
Before reading further, you should be familiar with DataLoader (also see this great video about the source code). You can also checkout the docs directory for some detailed examples of how the current implementation works and its benefits.
First things first, this package should be renamed to
feathers-dataloader
orfeathers-loader
. There is already a complimentary (yes, they are complimentary and not competitors) package feathers-batch. So to avoid confusion, this package should be renamed. For the remainder of this blurb, I will refer to the "BatchLoader" as "DataLoader" to reflect this change. Long story shortBatchLoader === DataLoader
.The Problem
Using dataloaders requires the developer to instantiate a new class every time they want to use a loader, and these classes require lots of config. This causes the developer to duplicate configuration (mainly local/foreign key relationships), It also causes the developer to know ahead of time which loaders are going to be used in any given request. For example,
This may not seem painful at first. Its explicit and can offer some great performance benefits. But, as you use more loaders and your loaders get more complicated the problem starts to get unwieldy. Because calls to
load()
are batched, params must be static. You cannot pass different params to each load. For example,Bummer, we had to create two loaders because params must be static within the class instance. This problems grows even further when you want to have variable params per
load()
call. This is not currently possible.The Solution
Create a syntax and pattern that makes using dataloaders easier and more approachable. Specifically, dataloaders should be lazily created as needed. This solution introduces two new classes
ServiceLoader
andLazyLoader
.The
ServiceLoader
lazily createsDataLoaders
for a particular service. It creates a newDataLoader
for any given set of id and params. This allows the developer to create one loader for a service, and then use any combination of local/foreign key relationships and params.The
LazyLoader
is a super simple class that lazily constructsServiceLoaders
.This accomplishes a very convenient, natural, service-like interface. The developer really doesn't need to know anything about ServiceLoaders, DataLoaders, etc, etc. If the developer knows how to use a service, they basically know how to use a loader.
Thats the general gist! I wanted to make dataloaders as convenient and approachable as possible and I think this mostly accomplishes that. I recently replaced my older implementations of loaders in a production app with this new version and it was a drop in replacement that went really well!
So some things I need feedback on
load()
method to take two different shapes. You can pass just an id, or an "id object".ServiceLoader
uses a deterministic stringify function to create keys for the underlying cache map. This also means that the developer cannot put functions into the params. I want to keep the experience as service-like as possible, but it is certainly different to split your params into two arguments. For example,Is there another solution where we combine the id object and query into one argument? The above solution works pretty well and makes sense to me, but I feel like someone out there may have a better solution.
get()
andfind()
methods. These have their use cases, but I don't want the classes to be too convoluted.I have found use cases for these. The
get()
method is almost always easily replaced withload()
, but there are some cases where you want to throw an error if id not found, or if the request must be aget
and not afind
(which load does under the hood) for some hooks/auth reason. The cachedfind()
definitely has value because there are cases where you simply don't have a local/foreign key relationship, but can still benefit from cachingBonus thoughts
This gets really exciting/convenient when considering feathersjs/feathers#2074.
Conclusion
Thanks for reading all this! Any and all feedback is welcome! I would specifically like some feedback on the 3 questions above, but anything else is definitely appreciated too.
The text was updated successfully, but these errors were encountered: