Remove filtering from model map

[blakeembrey]

Resolves #282. It seems this is built up elsewhere so there's no need to filter here. I ran into the issue of export type Foo = ... being omitted from the generated output.

(If this is not desirable, calling isEnum() as a function should resolve the type issue, but I wasn't sure why you'd want to filter here - the TypeScript type system could handle any errors for you).

[Weakky]

Hey, thanks for the PR!

There is indeed an issue here, but that doesn't have to do with the filter.
The filter is here to prevent the TS enums from being imported, as they're already inlined from the GraphQL schema.

The problem here is that .isEnum is a function and not a property.

This is the right fix:

return !(
  modelDef.kind === 'TypeAliasDefinition' &&
  (modelDef as TypeAliasDefinition).isEnum() // <-- .isEnum()
)

EDIT: I didn't see your edited comment! See above for the reasons why we're filtering 🙌

[blakeembrey]

Why would you care about the enum at this point though? Are you saying you copied it into the file elsewhere? In which case, it’s a little confusing to mix the logic for that into multiple places - can we just subtract those already imported types before running this function instead of spreading the filter across multiple files?

[blakeembrey]

Is https://github.com/prisma/graphqlgen/blob/26064174625c27965c99372235f653398c43d402/packages/graphqlgen/src/introspection/ts-ast.ts the right place to do that (e.g. avoid the doubling into enum and type map)?

[Weakky]

No, /packages/graphqlgen/src/introspection/ts-ast.ts isn't the right file to modify ! Enums are treated as "scalars", which means we always, always take the enums from your GraphQL schema and "copy/paste" them at the top of your graphqlgen.ts file without looking at your TS file.

This is done here: https://github.com/prisma/graphqlgen/blob/2606417462/packages/graphqlgen/src/generators/common.ts#L259-L267

Given that GraphQL schema:

type User {
  color: Color!
}

enum Color {RED GREEN BLUE}

Your graphqlgen.ts file will roughly look like this:

import { User } from 'your-types'

type Color = 'RED' | 'GREEN' | 'BLUE'

The enum will NEVER be imported from your ts types.

BUT, we still use the enums from your TS types (your model definitions) to decide whether we can or cannot scaffold the resolver of that enum field as a defaultResolver.

This is done here: https://github.com/prisma/graphqlgen/blob/2606417462/packages/graphqlgen/src/generators/common.ts#L167-L175 and you can find more info about that here: #244

For that reason, when importing the model definitions, we need to filter the enums from your types, to make sure we do not end up in such a situation:

import { User, Color } from 'your-types'

type Color = 'RED' | 'GREEN' | 'BLUE' // <- Duplicated type here

Does that make sense to you ?

[blakeembrey]

@Weakky I think it makes sense once I looked at #244. I did want to understand the design a bit better though - is modelMap a mapping of all TypeScript types to GraphQL types and not a map of just the models? Based on the code you pointed me to, it'd find the common properties between a TypeScript type and the GraphQL type so the filtering here doesn't appear to do much. I guess I'd need to play around with it a bit more to understand if what I just asked makes sense.

[blakeembrey]

@Weakky Is https://github.com/prisma/graphqlgen/blob/dc737fe4f3262e3fd00c23a14f0227a4a950deb4/packages/graphqlgen/src/tests/typescript/basic.test.ts#L22-L35 the test you expect to fail with this change?

[Weakky]

The typesMap is the map of all the model definitions per files, and the modelMap is indeed a map of all the model definitions that maps to a graphql type.

For the reasons stated above, we cannot filter the enums during the process of building that modelMap either (in parse.ts) because we still need to make comparisons between the enums properties later during the code generation.

However, we want to make sure that we never import the enums from your TS types because we always take the enums from your graphql schema as reference and as return type of the resolvers.

The test you showed me, despite being very light for the moment, is indeed the test that makes sure we properly render enums

[blakeembrey]

@Weakky So that test works. I'll try to add a test that disproves this, but I'm convinced you don't want/need the filter. Let me know if you already have a test for this?

[blakeembrey]

@Weakky I added a test that demonstrates the concern I've been trying to say. Hopefully it makes sense. Basically, if you have a model that is actually an enum here, you don't want to be filtering it out. That would cause the generated TypeScript file to be invalid.

Edit: Sorry for the slightly janky example using Permissions instead of Role. Permissions works in this example I come up with because if you use Role, you end up needing to do type Permissions = Role to accept the model and isEnum() would be false (even though it's an alias of an enum).

[ksaldana1]

I went down the same rabbit hole as @blakeembrey and came to the same conclusion. This is omitting type aliases from imports, making TS files invalid. I haven't looked into the enum PR here to know how all that interacts, but I don't think this filter is without its problems. Going to try to play with some more scenarios before saying anything definitive.

[blakeembrey]

Is there anyway we can land this feature? I can rebase if so, it's been blocking a few projects I'm involved in for a while 😄 I believe I demonstrated two bugs present with the filter, 1. it shouldn't filter and 2. the filter is broken.

[jasonkuhrt]

@blakeembrey go for it. It would be nice if you include a test that fails on master but passes on your branch.

[blakeembrey]

@jasonkuhrt That is/was already here 😦 I'll see about revisiting the diff another time.