Add configuration option for batch keys that are a Set rather than $ReadOnlyArray #265

kkellyy · 2021-05-13T23:05:59Z

Problem

There are cases where a batch key might not be an Array collection type, but rather a Set type. In these cases, flow needs to define the type of newKey differently. Currently, a function that defines a batch endpoint with a Set batch parameter will fail.

Solution

Allow users to set isBatchKeyASet: true to change the way the generated dataloader determines the type for newKey to work with a Set.

Here is a simple example of this typing working in the flow repl. This solution relies on Set<T>.has(), which accepts a parameter of type T.

Verification

make test passes. I've added some unit tests to verify it is behaving properly.
I also added a contrived example to examples/swapi.

gyDBD

testing in swapi looks good!

gyDBD · 2021-05-14T00:38:48Z

schema.json

@@ -93,6 +93,10 @@
                "isResponseDictionary": {
                    "type": "boolean",
                    "description": "(Optional) Set to true if the batch resource returns the results as a dictionary with key mapped to values (instead of a list of items). If this option is supplied `reorderResultsByKey` should not be. Default: false"
+                },
+                "uniqueBatchKeys": {


uniqueBatchKeys confused me a little bit. (uniqueBatchKeys makes me feel I should input what's the unique batch keys, not whether batchkey is unique.) would something like isBatchKeyASet be better?
and I like the description of commaSeparatedBatchKey a lot, so maybe we could follow that and do

"description": "(Optional) Set to true if the interface of the resource takes the batch key as a set (rather than an array as is more common). Default: false"

Great suggestion. I was really struggling with what to call this option haha. I like yours better for sure.

gyDBD

LGTM, cc @ATRAN2

magicmark · 2021-05-14T18:28:03Z

src/genTypeFlow.ts

+        let newKeyType = `${resourceConfig.newKey}: $ElementType<$PropertyType<${resourceArgs}, '${resourceConfig.batchKey}'>, 0>`;
+
+        if (resourceConfig.isBatchKeyASet) {
+            newKeyType = `${resourceConfig.newKey}: ${getNewKeyTypeFromBatchKeySetType(
+                resourceConfig.batchKey,
+                resourceArgs,
+            )}`;
+        }


may be worth a comment explaining "batchKey could be part of an array or a set" or something to explain what's going on here in the default / branching case?

magicmark · 2021-05-14T18:35:42Z

src/genTypeFlow.ts

+    return `\
+        $Call<
+            ExtractArg,
+            [$PropertyType<$PropertyType<${resourceArgs}, '${batchKey}'>, 'has'>]


doesn't has return a boolean rather than the underlying value?

https://github.com/facebook/flow/blob/388ea046bb7ca6f64fda17637daae8a3440ae0b2/lib/core.js#L600

slightly confused here sorry!

Yeah, finding a solution here wasn't easy/obvious, even when I was pairing on it with @ATRAN2. This is extracting the arg type of has() (T) similarly to how the code extracts the batchKey arg type to the batch resource function (ie Set<T> in this case) itself.

ohhhh this is the type of the newKey, sorry i'm being dumb

looks good, nice approach :) 👍

magicmark

looking good!

Could you build the swapi examples and commit the newly generated files?

(should add this step to the contribution guide)

https://github.com/Yelp/dataloader-codegen/tree/master/examples/swapi

would like to see the generated output of this

ATRAN2 · 2021-05-14T18:58:04Z

schema.json

+                },
+                "isBatchKeyASet": {
+                    "type": "boolean",
+                    "description": "(Optional) Set to true if the interface of the resource takes the batch key as a set (rather than an array). Default: false"


Might help to also note in the description what swagger flag can cause this to need to be True, so maybe a
when uniqueItems: true in the swagger spec

kkellyy · 2021-05-14T20:22:34Z

looking good!

Could you build the swapi examples and commit the newly generated files?

(should add this step to the contribution guide)

https://github.com/Yelp/dataloader-codegen/tree/master/examples/swapi

would like to see the generated output of this

It already is, no? Or is there more I needed to check-in?

magicmark · 2021-05-14T21:21:21Z

It already is, no? Or is there more I needed to check-in?

ah missed this, thanks!

We don't really have tests for the flow types here I don't think - but we can use the swapi example. Could we add a resolver that fetches something using this new option, and verify that flow doesn't blow up?

thanks!!

kkellyy · 2021-05-14T22:50:13Z

It already is, no? Or is there more I needed to check-in?

ah missed this, thanks!

We don't really have tests for the flow types here I don't think - but we can use the swapi example. Could we add a resolver that fetches something using this new option, and verify that flow doesn't blow up?

thanks!!

Done! also dataloader-codegen/examples/swapi (unique_set_batchkeys) $ node_modules/.bin/flow check returns 0 errors, so flow seems happy. If I switch to a string, it errors properly as well:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ swapi-server.js:81:74

Cannot call swapiLoaders.getFilms.load with object literal bound to key because string [1] is incompatible with
number [2] in property film_id.

     swapi-server.js
     78│         }
     79│
     80│         async title() {
 [1] 81│             const response = await swapiLoaders.getFilms.load({ film_id: 'foo' });
     82│
     83│             if (response instanceof Error) {
     84│                 return response;

     swapi.js
 [2] 81│     getFilms: ({| film_ids: Set<number> |}) => Promise<$ReadOnlyArray<SWAPI_Film>>,

Found 1 error

kkellyy · 2021-05-14T22:53:10Z

Oh also output from swapi-server.js:

dataloader-codegen/examples/swapi (unique_set_batchkeys*) $ node build/swapi-server.js                                                130 ↵
{
    "data": {
        "alderaan": {
            "name": "Alderaan",
            "climate": "temperate",
            "diameter": "12500"
        },
        "hoth": {
            "name": "Hoth"
        },
        "dagobah": {
            "name": "Dagobah"
        },
        "meh": {
            "title": "A New Hope",
            "episodeNumber": 4,
            "director": "George Lucas"
        },
        "theBest": {
            "title": "The Phantom Menace",
            "episodeNumber": 1,
            "director": "George Lucas"
        }
    }
}

magicmark

Tidy work!

Love the links to tryflow and flowdefs in the PR description, helped me a lot 👍

magicmark · 2021-05-14T23:02:52Z

Could you update the docs too? https://github.com/Yelp/dataloader-codegen/blob/HEAD/API_DOCS.md

magicmark

lg2m!

kkellyy added 2 commits May 13, 2021 15:21

Set type of newKey when batch key type is a Set instead of an Array

1d1794b

Add example endpoint with Set parameter and uniqueBatchKeys: true

c888d9e

kkellyy requested review from magicmark and gyDBD May 13, 2021 23:05

gyDBD reviewed May 14, 2021

View reviewed changes

Rename config option to more precise name

9039ed0

kkellyy requested a review from gyDBD May 14, 2021 17:20

gyDBD approved these changes May 14, 2021

View reviewed changes

magicmark reviewed May 14, 2021

View reviewed changes

ATRAN2 reviewed May 14, 2021

View reviewed changes

ATRAN2 approved these changes May 14, 2021

View reviewed changes

Add some comments to new code and provide example usecase of new option

e2dbb6a

Add a film resolver and some "fun" queries to test that it works

6f6af77

kkellyy requested a review from magicmark May 14, 2021 22:43

magicmark approved these changes May 14, 2021

View reviewed changes

update API_DOCS.md as well

0b23092

magicmark approved these changes May 14, 2021

View reviewed changes

kkellyy merged commit 0aaa906 into Yelp:master May 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration option for batch keys that are a Set rather than $ReadOnlyArray #265

Add configuration option for batch keys that are a Set rather than $ReadOnlyArray #265

kkellyy commented May 13, 2021 •

edited

Loading

gyDBD left a comment

gyDBD May 14, 2021 •

edited

Loading

kkellyy May 14, 2021

gyDBD left a comment

magicmark May 14, 2021

magicmark May 14, 2021

kkellyy May 14, 2021

magicmark May 14, 2021

magicmark left a comment

ATRAN2 May 14, 2021

kkellyy commented May 14, 2021

magicmark commented May 14, 2021

kkellyy commented May 14, 2021

kkellyy commented May 14, 2021

magicmark left a comment

magicmark commented May 14, 2021

magicmark left a comment

Add configuration option for batch keys that are a Set rather than $ReadOnlyArray #265

Add configuration option for batch keys that are a Set rather than $ReadOnlyArray #265

Conversation

kkellyy commented May 13, 2021 • edited Loading

Problem

Solution

Verification

gyDBD left a comment

Choose a reason for hiding this comment

gyDBD May 14, 2021 • edited Loading

Choose a reason for hiding this comment

kkellyy May 14, 2021

Choose a reason for hiding this comment

gyDBD left a comment

Choose a reason for hiding this comment

magicmark May 14, 2021

Choose a reason for hiding this comment

magicmark May 14, 2021

Choose a reason for hiding this comment

kkellyy May 14, 2021

Choose a reason for hiding this comment

magicmark May 14, 2021

Choose a reason for hiding this comment

magicmark left a comment

Choose a reason for hiding this comment

ATRAN2 May 14, 2021

Choose a reason for hiding this comment

kkellyy commented May 14, 2021

magicmark commented May 14, 2021

kkellyy commented May 14, 2021

kkellyy commented May 14, 2021

magicmark left a comment

Choose a reason for hiding this comment

magicmark commented May 14, 2021

magicmark left a comment

Choose a reason for hiding this comment

kkellyy commented May 13, 2021 •

edited

Loading

gyDBD May 14, 2021 •

edited

Loading