Make count fields plain integers.

[alloy]

TL;DR

In short, I would like us to use a plain GraphQLInt for counts fields. I don’t really see a need to be able to format a count in MP, but would like to know if others have objections.

---

As we move to TypeScript and convert MP’s schema to type declarations, we’re running into the issue that for many (all?) of the counts fields we have, the resulting type declaration is number | string | boolean, because we are using a custom scalar rather than a specific subtype.

This type declaration is problematic because it would require us to go through hoops to cast the value as a number in order to safely write code like:

const not_for_sale_works = artist.counts.artworks - artist.counts.for_sale_works

The TypeScript compiler will fail with the following error:

error TS2362: The left-hand side of an arithmetic operation must be of type 'any', 'number' or an enum type.
error TS2363: The right-hand side of an arithmetic operation must be of type 'any', 'number' or an enum type.

/cc @sweir27 @broskoski @l2succes

[alloy]

The field that actually uses this type is the numeral one.

[1aurabrown]

We do in fact take advantage of the string formatting and label parameters in a few places, just in force:

https://github.com/artsy/force/blob/master/apps/artwork/components/artist_artworks/query.coffee#L7 https://github.com/artsy/force/blob/master/apps/artwork/components/partner/query.coffee#L21 https://github.com/artsy/force/blob/master/apps/artwork/components/partner_artworks/query.coffee#L7

perhaps we should make a Type that has two fields, one for formatted numerals and one for raw Ints, so our queries would be like

counts {
  artworks {
    formatted(format: "0,0", label: "work") # -> 3 works
    raw # -> 3
  }
}

if we want to be able to use this capability

[1aurabrown]

as well as here: https://github.com/artsy/microgravity/blob/master/apps/artwork/components/related_artworks/queries/partner_query.coffee#L7

[mzikherman]

I'm sort of into having the counts be able to be formatted on Metaphysics, either by default or with some params. If we want the raw field as well that could be supported.

It seems likely that most clients will want the ability to use the counts in a 'sentence' (ie- 1 work, 2 works), with pluralization. Why not let Metaphysics do that?

I see the example you posted, but that seems like an oddly specific TypeScript/mobile issue to me :)

[1aurabrown]

We could also add a not_for_sale_works count field that does the math for not_for_sale_works = artist.counts.artworks - artist.counts.for_sale_works before calling numeral

[alloy]

@1aurabrown Good sleuthing 👌

perhaps we should make a Type that has two fields, one for formatted numerals and one for raw Ints, so our queries would be like

I like this idea, but I’m not sure how we’d migrate to it, as it would break existing use of those count fields completely. Alternative thoughts?

We could also add a not_for_sale_works count field that does the math for not_for_sale_works = artist.counts.artworks - artist.counts.for_sale_works before calling numeral

It wouldn’t really solve the issue in our case, which is to use the count to test whether or not we’ll show a component.

I’d like having this field in the schema regardless, though 👍

[alloy]

I see the example you posted, but that seems like an oddly specific TypeScript/mobile issue to me

@mzikherman It’s more about defining a clear API imo. Nonetheless, it’s going to be less of a mobile specific issue real soon in reaction-force 😉

[1aurabrown]

@alloy in the case of not_for_sale_works, if you requested that count without the format or value params, wouldnt the json response for that field just be an integer as far as the client is concerned? therefore if we did the math on metaphysics and had these same optional formatting params, you could request that count without those params and get an integer (as far as the client is concerned) to test if it is greater than 0 for the sake of including a component.

[alloy]

Maybe this will actually become less of a problem as we start adding more Relay connection fields, because, correct me if I’m wrong @broskoski, connections also provide metadata such as the total count of the connection as an Int.

[1aurabrown]

I'm confused -- what is the problem TypeScript would have with these values:

[broskoski]

@alloy Not automatically. pageInfo only returns cursors (startCursor and endCursor) as well as whether there are more page results or not. See: https://facebook.github.io/relay/graphql/connections.htm#sec-undefined.PageInfo

[alloy]

@1aurabrown Yeah you’re totally right, of course, but that’s an at runtime detail, the issue comes into play when we want a compiler to be able to know the exact type we expect a field to to return at compile-time.

For example, in our current untyped code, we just expect the values to be integers and it all works as expected. However, in our new typed code the schema that MP exports specifically tells the compiler that these fields can return a plethora of types, even when we know it won’t.

[1aurabrown]

yeah, i see i see.

[alloy]

Well, I can’t come up with a good solution, other than renaming the current fields to e.g. formatted_for_sale_artworks and re-type the existing for_sale_artworks field to plain Int.

I’m ok with letting this simmer and see if we can solve this otherwise in the client, unless other people think that added ‘formatted’ fields is a good way to go.

[broskoski]

+1 for formatted fields

[1aurabrown]

yeah, even that would be a breaking change for these clients because for_sale_artworks would be returning something different than the clients currently expect (id 3 instead of '3 works') so we'd have to time this all across clients

[broskoski]

To clarify, I'm for keeping the formatted fields.

[1aurabrown]

@broskoski what are your feelings on the matter ;)

[alloy]

@1aurabrown Yeah totally, the only difference is that we actually can control the deployment of the other clients (web apps), albeit a lot of orchestration work, but we cannot easily control what version of Eigen people have installed on their iPhone.

[broskoski]

I think it makes more sense to add a raw field that can be typed if we want to make things nice for TypeScript. There are still a lot of places in Force that use these formatted arguments, and making this stuff easier (i.e. de-duplicating simple formatting functionality) was (one of) the primary purpose(s) for building Metaphysics.

[alloy]

@broskoski Good point, we can just prefix the int fields. Maybe as another nested field? E.g.

counts {
  raw {
    for_sale_artworks
  }
}

Not a big fan of the word 'raw', though.