Shopify / graphql-js-schema

Transforms the JSON representation of a GraphQL schema into a set of ES6 type modules
MIT License
35 stars 12 forks source link

On optimizing type bundle size #8

Open minasmart opened 8 years ago

minasmart commented 8 years ago

Problem

Right now, we're repeating a lot of data. A lot of similar information appears on the field itself, and on the type's definition. There are also a couple other things we could blow away.

We could probably reduce things to the following:

const ProductConnection = {
  objects: {
    edges: 'ProductEdge',
    pageInfo: 'PageInfo'
  }
}

const QueryRoot = {
  objects: { ... },
  interfaces: ['Node'],
}

const Shop = {
  scalars: {
    createdAt: 'DateTime',
    countryCode: 'String',
    someOtherKey: 'Int',
  },
  objects: {
    products: 'ProductConnection',
    images: 'Image'
  }
}

With this subset, we'd only be able to deduce OBJECT or INTERFACE from SCALAR from inference. If the type isn't on the bundle, it's a scalar.

Comments and discussion welcome!

minasmart commented 8 years ago

@jamesmacaulay & @mikkoh

jamesmacaulay commented 8 years ago

an alternative for discussion:

const Shop = {
  "name": "Shop"
  "kind": "OBJECT",
  "fieldBaseTypes": {
    "description": "String",
    "moneyFormat": "String",
    "name": "String",
    "billingAddress": "Address",
    "primaryDomain": "Domain",
    "collections": "CollectionConnection",
    "products": "ProductConnection"
  },
  "implementsNode": false
}
minasmart commented 8 years ago

This is even more human readable. I like it.

minasmart commented 8 years ago

I'm going back and forth on how we infer SCALARS. Do you think it's safe to have the logic of "this type is not part of our bundle, therefore it's a scalar"? Or is there a value in differentiating an unknown type from a SCALAR?

jamesmacaulay commented 8 years ago

IMO there are such a small number of scalars, and they never change, we might as well include them in the bundle. Then we can easily say that anything not in the bundle is invalid.

mikkoh commented 8 years ago

I guess the only Scalar that is not defined in JS is ID. Since this is JS what if we did this?

const Shop = {
  "name": "Shop"
  "kind": Object,
  "fieldBaseTypes": {
    "description": String,
    "moneyFormat": String,
    "name": String,
    "billingAddress": Address,
    "primaryDomain": Domain,
    "collections": CollectionConnection,
    "products": ProductConnection
  },
  "implementsNode": false
}

How about lists?

const SomethingThatHasLists = {
  "name": "SomethingThatHasLists",
  "kind": Object,
  "fields": {
    "listStrings": [String],
    "listBoolean": [Boolean],
    "listAddresses": [Address]
  },
  "implementsNode": false
}

Non-null would be tough though.

minasmart commented 8 years ago

DateTime, and Money are also not defined in JS. We can also add arbitrary scalars to the schema.

I don't think we need the list definition, because that doesn't actually change the query? The only place that information is used is in deserializeObject, and we could really just do Array.isArray(object) at the same point in the branching logic.

mikkoh commented 8 years ago

With args do we do something like:

import connectionArgs import './defaults/connection-args';
import CollectionSortKeys import './CollectionSortKeys';

const Shop = {
  ...
}

if(process.env.NODE_ENV === 'development') {
  Shop.fieldArguments = {
    collections: {
      ...connectionArgs,
      reverse: Boolean,
      query: String,
      sortKey: CollectionSortKeys
    }
  };
}
jamesmacaulay commented 8 years ago

We can also add arbitrary scalars to the schema.

Oops, I didn't know that. I think that's extra reason to include all scalars in the type bundle then.

mikkoh commented 8 years ago

I think we need to handle args in production. Because when we build the query we need to format Objects and Enums when building a query.

Also we need to know for args if the arg accepts a list because if we have lets say a List of Enums. This being said we could only keep those types in the production bundle and assume everything else is a Scalar.

jamesmacaulay commented 8 years ago

Because when we build the query we need to format Objects and Enums when building a query.

What formatting needs to be done? The args are written as a POJO and no extra type information is needed in order to serialize it.

minasmart commented 8 years ago

I've got the same curiosity as James. The more we talk about it, the more I feel like args are something we can punt on for a long time. At the moment, I don't think there's any validation (other than arg exists, or arg is required) that we can do well on the client. And even exists/required constraints don't give us much, because if the value isn't correct, the server will reject it anyway.

@lreeves what are you storing in regard to args in the ruby client?

lreeves commented 8 years ago

We store and validate args. Connections all require an argument named first for example and being able to validate that it's present pre-network seems pretty useful. Product#images is a field in the StoreFront API that accepts complex arguments as well. You can probably ignore them for a while but client-side validation is a pretty nice to have feature.

minasmart commented 8 years ago

In graphql-ruby-schema what do you persist in regard to args? What file can I look at? Do you do validation of complex types, or just validate required are present?

mikkoh commented 8 years ago

Yep of course we can do:

if(Array.isArray(argValue)) {
  LOOP THROUGH EACH ELEMENT AND FORMAT RECURSIVELY
} else if(typeof argValue === 'object') {
  POJO
} else {
  JUST JSON.stringify
}

How do we handle Enums?

addField('something', {enum: 'THIS_IS_AN_ENUM'})

Should produce:

{
  something(enum: THIS_IS_AN_ENUM)
}

not

{
  something(enum: "THIS_IS_AN_ENUM")
}

Do we use Symbol for Enums instead of a String?

addField('something', {enum: Symbol('THIS_IS_AN_ENUM')})

Although... If we use Symbols when going to es5 Babel will need to drop in typeof2. Which will increase bundle size.

Because:

typeof Symbol('mikko') === 'symbol'

Will not work in es5.

lreeves commented 8 years ago

I'm not sure what you quite mean by persist, but we load everything that we can from the schema into memory and map input types with the usual wrapping information (NULL/non-NULL) stuff on arguments. Validation of correctly named arguments happens:

https://github.com/Shopify/graphql-ruby-client/blob/master/lib/graphql_client/query/query_field.rb#L52

The query builder in the client does the client-side validation of args. You can also inspect all of the arguments after the schema is loaded:

@client.schema.types['Product'].fields['images'].args
=> {"first"=>
  #<GraphQLSchema::Types::InputValue:0x007f9de812a060
   @base_type=
    #<GraphQLSchema::Types::Scalar:0x007f9de2f92090
     @data={"kind"=>"SCALAR", "name"=>"Int", "description"=>"Represents non-fractional signed whole numeric values. Int can represent values between -(2^31) and 2^31 - 1.", "fields"=>nil, "inputFields"=>nil, "interfaces"=>nil, "enumValues"=>nil, "possibleTypes"=>nil},
     @schema=#<GraphQLSchema::Schema:0x3fcef175ca74>>,
   @data={"name"=>"first", "description"=>nil, "type"=>{"kind"=>"NON_NULL", "name"=>"Non-Null", "ofType"=>{"kind"=>"SCALAR", "name"=>"Int", "ofType"=>nil}}, "defaultValue"=>nil},
   @schema=#<GraphQLSchema::Schema:0x3fcef175ca74>,
   @type=
    #<GraphQLSchema::Types::NonNull:0x007f9de8129c28
     @data={"kind"=>"NON_NULL", "name"=>"Non-Null", "ofType"=>{"kind"=>"SCALAR", "name"=>"Int", "ofType"=>nil}},
     @of_type=
      #<GraphQLSchema::Types::Scalar:0x007f9de2f92090
       @data={"kind"=>"SCALAR", "name"=>"Int", "description"=>"Represents non-fractional signed whole numeric values. Int can represent values between -(2^31) and 2^31 - 1.", "fields"=>nil, "inputFields"=>nil, "interfaces"=>nil, "enumValues"=>nil, "possibleTypes"=>nil},
       @schema=#<GraphQLSchema::Schema:0x3fcef175ca74>>,
     @schema=#<GraphQLSchema::Schema:0x3fcef175ca74>>,
   @wrapping_types=
    [#<GraphQLSchema::Types::NonNull:0x007f9de8129c28
      @data={"kind"=>"NON_NULL", "name"=>"Non-Null", "ofType"=>{"kind"=>"SCALAR", "name"=>"Int", "ofType"=>nil}},
      @of_type=
       #<GraphQLSchema::Types::Scalar:0x007f9de2f92090
        @data={"kind"=>"SCALAR", "name"=>"Int", "description"=>"Represents non-fractional signed whole numeric values. Int can represent values between -(2^31) and 2^31 - 1.", "fields"=>nil, "inputFields"=>nil, "interfaces"=>nil, "enumValues"=>nil, "possibleTypes"=>nil},
        @schema=#<GraphQLSchema::Schema:0x3fcef175ca74>>,
      @schema=#<GraphQLSchema::Schema:0x3fcef175ca74>>]>,

In the above example it's the first field and we have data concerning the fact that it's required and an integer and so forth. So it's basically the full definitions there, validating what we can against them. Does that help?

minasmart commented 8 years ago

You're right about enums being hard to serialize. We may want to expose a simple function that just wraps the value, like...

addField('images', { crop: enum('CENTER') })

function enum(value) {
  const str = new String(value);
  str.isEnum = true;
  return str;
}

That'd be way more terse than using Symbol, and we'd still capture the information. We'd just need to ensure we always call toString on any value object.

Provided we have a sane way of passing enums, I still think it may be manageable to punt on this feature for a while.

I am, however, still interested in how we might build the values required for validation into our type bundle.

jamesmacaulay commented 8 years ago

How do we handle Enums?

Sorry, I didn't realize enums weren't quoted in args. Having an enum constructor of some kind would be my vote.

jamesmacaulay commented 8 years ago

Proof of concept implementation for my initial proposal above: https://github.com/Shopify/graphql-js-schema/pull/10