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.

Sign up for GitHub

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

Validate shape of cacheMap on initialization #86

Merged
merged 6 commits into from
Oct 19, 2017
Merged

Validate shape of cacheMap on initialization #86

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e323ec6
Validate shape of cacheMap on initialization
sidsethupathi Jun 15, 2017
a255b07
Follow up review:
leebyron Oct 19, 2017
25800d4
Update abuse-test.js
leebyron Oct 19, 2017
553eb16
map().filter() can be replaced by .filter()
leebyron Oct 19, 2017
a8e5c32
80ch
leebyron Oct 19, 2017
f020aed
Flow
leebyron Oct 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/__tests__/abuse-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,17 @@ describe('Provides descriptive error messages for API abuse', () => {
);
});

it('Cache should have get, set, delete, and clear methods', async () => {
class IncompleteMap {
get() {}
}

expect(() => {
var incompleteMap = new IncompleteMap();
var options = { cacheMap: incompleteMap };
new DataLoader(keys => keys, options); // eslint-disable-line no-new
}).to.throw(
'Custom cacheMap missing methods: set, delete, clear'
);
});
});
21 changes: 19 additions & 2 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ export default class DataLoader<K, V> {
}
this._batchLoadFn = batchLoadFn;
this._options = options;
this._promiseCache =
options && options.cacheMap || (new Map(): Map<K,Promise<V>>);
this._promiseCache = getValidCacheMap(options);
this._queue = [];
}

Expand Down Expand Up @@ -305,6 +304,24 @@ function failedDispatch<K, V>(
});
}

function getValidCacheMap<K, V>(
options: Options<K, V>
Copy link
Author

Choose a reason for hiding this comment

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

I'm pretty new to flow, but shouldn't this be options??

): CacheMap<K, Promise<V>> {
var cacheMap = options && options.cacheMap;
if (!cacheMap) {
return new Map();
}
var cacheFunctions = [ 'get', 'set', 'delete', 'clear' ];
var missingFunctions = cacheFunctions
.filter(fnName => typeof cacheMap[fnName] !== 'function');
if (missingFunctions.length !== 0) {
throw new TypeError(
'Custom cacheMap missing methods: ' + missingFunctions.join(', ')
);
}
return cacheMap;
}

// Private
type LoaderQueue<K, V> = Array<{
key: K;
Expand Down