graphql-nexus / nexus

Code-First, Type-Safe, GraphQL Schema Construction
https://nexusjs.org
MIT License
3.4k stars 274 forks source link

Global TypeScript interface type for easy extension of model typings #586

Open jasonkuhrt opened 3 years ago

jasonkuhrt commented 3 years ago

Currently configuring the model typings of an app usually requires going through the typegenAutoConfig.backingTypeMap. This can be cumbersome. Especially for small ad-hoc changes. Another method could be offered to users that might serve some use-cases better. The idea would be a global interface type which can then be extended and once so, would be reflected in the map typings across the codebase.

The example below uses the new proposed model typings api (#583).

Example:

declare global {
 interface NexusModels {
   Foo: {
     a: string
     b: number
   }
 }
}

objectType({
  name: "Thing",
  modelTyping: "Foo",
  definition(t) {
    t.field('qux', {
      resolve(foo) {
        foo.a.toLowerCase() // type checks
        foo.b.toFixed() // type checks
      }
    })
  }
})
Weakky commented 3 years ago

A quick point here: there should be no need to specify modelTyping at all. I think what you meant is:

interface NexusModels {
  Thing: {
    a: string
    b: number
  }
}

objectType({
  name: "Thing",
  definition(t) {
    t.field('qux', {
      resolve(thing) {
        thing.a.toLowerCase() // type checks
        thing.b.toFixed() // type checks
      }
    })
  }
})

Correct me if you see it differently

Weakky commented 3 years ago

Additional question(s):

jasonkuhrt commented 3 years ago

Correct me if you see it differently

If the Object name matches the Model name then there is no need to explicitly specify. Otherwise there is. Right?

Do we want to remove the typegenAutoConfig.backingTypeMap config? (I think we should)

Not sure. Are there no use-cases for it after this feature lands?

Does this has precedence over the typegenAutoConfig.sources config? (I think it should)

Yes definitely if we think of NexusModels as being only in the users application. However if users start to ship plugins/libs with it (like we did in Nexus Framework) then it seems to be less clear.

I think at least for now we should try having NexusModels take precedence 👍 .

Nayni commented 3 years ago

I like the idea of having the backing type map and sources more in user land. However I'd like to show an example of why I would be against removing sources in their current state.

In most of the projects I work on I try to strive for convention over configuration. One of the ways this manifests is that I advocate for strong conventions around files and folders. An example of such a convention is that it should be clear where database or domain models are coming from.

A real example of a project I work on is the following:

Having these two conventions in place, wiring up Nexus for the 90% use-case is trivial, we have 1 function which does all of it based on file conventions:

/**
 * Search all modules for possible Entities
 * (based on file convention, files that contain entities should end with Entity).
 * For all these entities it creates a typegen source for Nexus to inject.
 */
function getEntitySources() {
  const modules = findModuleDirectories();
  return modules
    .flatMap(m => fs.readdirSync(m).map(f => path.join(m, f)))
    .filter(f => fs.statSync(f).isFile() && ENTITY_FILE_REGEX.test(f))
    .map((f, i) => {
      return {
        source: f,
        alias: (
          ENTITY_FILE_REGEX.exec(f)?.groups?.entity || `entity${i})`
        ).toLowerCase(),
      };
    });
}

This auto wires up any entity that is created as a possible backing type for Nexus to match on. Allowing anyone to introduce a new entity without having to worry about schema config.

The global interface would be nice for our use-case but only solves for those 10% edge cases we wire up manually...

jasonkuhrt commented 3 years ago

@Weakky and I talked about this today. We decided that the lack of type safety (e.g. typos) when populating:

declare global { interface NexusModels { ... } }

is a significant problem. We decided to keep this in discussion phase for longer before implementing.

Meanwhile there are other potential solutions for making model type maps easier to work with in Nexus.

We'll try to post a summary post soon, maybe as a new broad issue called "make model type mapping easier".

tgriesser commented 3 years ago

We decided that the lack of type safety (e.g. typos) when populating:

Are you saying that someone might misspell NexusModels as NexusModel or something?

jasonkuhrt commented 3 years ago

@tgriesser didn't even think of that case. Others though like:

declare global { interface NexusModels { maybeTypoHere: SomeUserType  } }
tgriesser commented 3 years ago

I see. Hmm. I wonder if there's some way to make an invariant type which maps keyof NexusModels and ensures that maybeTypoHere is indeed a valid type name?

jasonkuhrt commented 3 years ago

there's some way to make an invariant type

@Weakky and I couldn't think of a way. If we used a generic we could enforce the keys of the passed type, e.g. NexusModels<{...}> but I don't think this could work with interface declaration merging.

I could see how we could create a type within typegen that does this check and will lead to a type error if there are any typos. However far from perfect: 1) very unclear feedback for users 2) I don't think users would see this if skipLibCheck is enabled and they are generating typegen into node_modules e.g. node_modules/@types/foo/index.d.ts.

Sytten commented 3 years ago

I also agree I like sources but they lack tooling. For my projects, I usually use a regex to match the name with the prisma generated ones and I can overwrite them if I create a type with the same name in my models folder index.ts. There is also one more source that wasn't discussed, the custom scalars backing types. If feel this is generally what is causing trouble for beginners.