General development experience questions and possible improvement points

[victorhmp]

Hey everyone 👋🏻 !

These are a few points that caught my attention when working on the @faststore/api package that I think are worth discussing to improve our developer experience, both for us and our users.

General points

1. This is more of a question, but is there a specific reason to why the getSchema function is an async function? It's just a wrapper of the makeExecutableSchema function, which is not async, so doesn't return a Promise. We're always returning a Promise<GraphQLSchema>, but I don't see why we need to do so. This makes the API just a bit annoying to use because most of the time the user will have to await the call to getSchema on their server code (just as we do on base.store and on the local Express server recently added to the package) and create and call a wrapper async function just to do so. Given this, can we make getSchema a synchronous function?

2. I don't think we're taking full advantage of TypeScript when it comes to our resolvers. At packages/api/src/platforms/vtex/index.ts we create and export a type called Resolver, defined as:

type Resolver<R = unknown, A = unknown> = (
  root: R,
  args: A,
  ctx: Context,
  info: any
) => any

First, I think more descriptive names for the type arguments would improve readability for us and for users. Maybe something like type Resolver<Root = unknown, Arguments = unknown>. Second, we're always typing the return for all resolvers as any here. And that doesn't help us to verify that our resolvers are actually returning what they should. I suggest we add a third generic argument to Resolver, something along the lines of type Resolver<Root, Arguments, Return>, so that we can correctly type check all resolvers. And since this is exported from the package, we would also be encouraging our users to do the same with their custom resolvers.

3. Kinda related to the previous point, but I also noticed that in the file packages/api/src/platforms/vtex/resolvers/aggregateOffer.ts, we're redefining and using another Resolver type, which doesn't extend or use the one cited in previously in any way. And this "new" Resolver type has its return typed as unknown, and I don't think we should keep it that way, but correctly type the expected return so that TypeScript helps us a bit more.

4. I believe the fact that in all of our resolvers we're defining inline functions inside of an object that is exported hurts readability. There are some pretty complex field resolvers among other simpler ones, but having them all defined inside a single object might be confusing, and I think quite a few of them would be easier to read and understand if they were extracted to their own functions, defined outside of the main object and then just referenced. A good example of this would be the Query object, which can be found at packages/api/src/platforms/vtex/resolvers/query.ts. This object became quite big, and there are some more complex resolvers inside of it, such as the allCollections one.

5. I believe we should take further advantage of the awesome GraphQL tooling that exists and add comments to our Schema. Tools that help users write their own queries and mutations (like GraphiQL and GraphQL Playground) usually read comments in a schema and show them as nice formatted documentation to users. We currently don't have any comment in the schema we're exporting from @faststore/api, but I really think we would be helping our users if we added them in all queries, mutations, and types.

Specific points

The following are way more specific and usually affect bits of certain files. Mostly focused on making the code easier to read.

packages/api/src/platforms/vtex/resolvers/collection.ts

1. There's an isBrand function that is being used to determine if an object (named x in the function definition) has the property type set to 'brand'. I don't think we should use variable names such as x or y or anything like that anywhere in the code. And specifically in this function, we receive x as any, and don't check its type at all. So if you call isBrand(null) for example, TypeScript will not complain, and the code would throw when trying to access x.type.

2. Inside the exported resolver object, the field of type is set to

type: (root) =>
    isBrand(root)
      ? 'Brand'
      : isCollectionPageType(root)
      ? root.pageType
      : root.level === 0
      ? 'Department'
      : 'Category',

And I think we should avoid chaining ternary operations like this to also improve readability.

packages/api/src/platforms/vtex/resolvers/product.ts

1. I think we should move the following comments

// This error will likely happen when you forget to forward the channel somewhere in your code.
// Make sure all queries that lead to a product are forwarding the channel in the context correctly.

into the actual error message we're sending to users because they're very likely not going to see this comment here.

packages/api/src/platforms/vtex/resolvers/query.ts

1. I think it's worth it to add a comment inside the allCollections resolver explaining why we're running a DFS on the category tree.

2. On this bit of code

const collections = [
      ...brands
        .filter((brand) => brand.isActive)
        .map((x) => ({ ...x, type: 'brand' })),
      ...categories,
    ]

I don't think we should use x as a variable name here, even though it's just inside a map, it only hurts readability.

3. I notice that we're calling a slugify function that comes from the StoreCollection resolver inside this the Query resolver, and I don't think we should be doing that. Instead, we could just move the slugify function into the utils folder. Oh! I also notice that we already have a slugify function in there, but we're using a different one here. This is something that might also hurt readability.

packages/api/src/platforms/vtex/resolvers/searchResults.ts

1. We should justify the REMOVED_FACETS_FROM_COLLECTION_PAGE variable, especially because its value is an array with a single value, which is a string in Portuguese ('departamento').

2. Inside the actual resolver, we're using the client for the Search API. But we're calling it is inside of this function, which I understand is the abbreviation of IntelligentSearch, but I don't think we should do so. It might not be instantly clear to everyone that is means IntelligentSearch, especially when we use is instead of IS. I suggest we change this to be more descriptive, by renaming it to something like intelligentSearch.

My totally biased and personal opinion is that we should never use abbreviations in the codebase. It doesn't make the code simpler and tends to only hurt readability.

packages/api/src/platforms/vtex/loaders/sku.ts

1. There are two errors that can potentially be thrown from this loader, and I think we should make their messages a bit more descriptive.

packages/api/src/platforms/vtex/loaders/collection.ts

1. There's an isCollectionPageType function defined in this file that suffers from the same problems that the isBrand function does. So I think we should also change this function to be saffer and more descriptive.