Skip to content

Conversation

@magicmark
Copy link
Collaborator

@magicmark magicmark commented Apr 13, 2020

Oops! We forgot to apply custom middleware to non batch resources.

What's going on here?

dataloader-codegen works by having the user define a config file of "resources" [1] that they want to generate dataloaders for.

[1] A "resource" = a function that returns data - e.g. a Promise from a call to fetch. In Yelpy terms, this is an API function exported by a node clientlib

A config file might look like this:

resources:
getPlanets:
docsLink: https://swapi.co/documentation#planets
isBatchResource: true
batchKey: planet_ids
newKey: planet_id

You can see that we declared getPlanets to be a batch resource, by passing isBatchResource: true.

dataloader-codegen has two seperate codegen methods - one for batch resources, and one for non batch resources:

What's "middleware"?

Functions that run before and after the call to the underlying resource - allowing the user to globally transform the arguments/response before passing it back to userland.

Our generated code should wrap the call to the underlying resource with middleware, rather than calling the underlying resource directly.

See: https://github.com/Yelp/dataloader-codegen/blob/master/API_DOCS.md#getloaders-arguments

What's the problem?

The codegen for batch resources and non-batch resources are separate methods (see above) and share no common code. When we added support for middleware to this library, we never added it to the getNonBatchLoader codegen 😬

What does this PR do?

Fixes the above issue, by pulling the middleware wrapped call to the underlying resource out into a seperate codegen method, to be called by both getNonBatchLoader and getBatchLoader.

Comment on lines 420 to 425
if (typeof ${resourceReference} !== 'function') {
return Promise.reject([
'${errorPrefix(resourcePath)} ${resourceReference} is not a function.',
'Did you pass in an instance of ${resourcePath.join('.')} to "getLoaders"?',
].join(' '));
}
Copy link

Choose a reason for hiding this comment

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

Maybe this should this be a regular throw? It can be a little confusing to mix Promises and async/await in the same function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ya we can do that (should be functionally equivalent)

…urces

- Specify cache key (string) to DataLoader type to fix flow errors
@magicmark magicmark force-pushed the fix_error_handling_non_batch_resources branch from 7c92cc8 to 2ae5b98 Compare April 16, 2020 20:38
@magicmark magicmark merged commit 97a73bb into master Apr 16, 2020
@magicmark magicmark deleted the fix_error_handling_non_batch_resources branch April 16, 2020 22:02
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.

4 participants