Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to handle overrides when importing? #203

Open
Siyfion opened this issue Jul 20, 2018 · 24 comments
Open

How to handle overrides when importing? #203

Siyfion opened this issue Jul 20, 2018 · 24 comments

Comments

@Siyfion
Copy link

Siyfion commented Jul 20, 2018

Firstly, I have an example reproduction of the issue I'm facing available here: https://github.com/Siyfion/node-graphql-starter/tree/graphql-import

So what I'm trying to do, is get my Prisma generated schema imported into my Apollo Server 2.0 project. It works fine doing it the way I am, until I re-define a type that's indirectly being imported with graphql-import.

Clearly I need a way of merging the schema and when encountering duplicates, using the ones defined in my code over the imported ones, but I can't seem to figure out a way to do this?

FYI the error in the example repo is: Error: Type "User" was defined more than once.
This is due to me having defined a custom type for User that adds a fullname field to the original schema.

@divyenduz divyenduz self-assigned this Jul 20, 2018
@marktani
Copy link

Can you still reproduce this with graphql-yoga and/or apollo-server@1.x?

@Siyfion
Copy link
Author

Siyfion commented Jul 20, 2018

@marktani I'm not 100% certain if I can or can't... One of the reasons for me moving to an Apollo Server 2.0 was to better organise my code, create a more modular based structure. I've had it working in graphql-yoga but only when I've had my import and entire schema all in one massive *.graphql file.

@Siyfion
Copy link
Author

Siyfion commented Jul 31, 2018

So as a bit of extra information, it all seems to work fine if I do the following:

  • Remove the type definitions from user.js and venue.js
  • Add the type definitions to imports.graphql

Resulting in:

# import List from './generated/prisma.graphql'
# import ListItem from './generated/prisma.graphql'
# import Menu from './generated/prisma.graphql'
# import OpeningHours from './generated/prisma.graphql'
# import Tag from './generated/prisma.graphql'
# import VenueWhereInput, VenueOrderByInput from './generated/prisma.graphql'
# import Zone from './generated/prisma.graphql'

type User {
    id: ID!
    activities: [ActivityEvent!]!
    areas: [Area!]
    email: String
    firstName: String
    following: [User!]
    followedLists: [List!]
    fullName: String
    username: String
    lastName: String
    lists: [List!]
    location: String
    metadata: Json
    profileImage: String
    tags: [Tag!]!
    zones: [Zone!]!
  }

  type Venue {
    id: ID!
    name: String!
    shortDescription: String!
    longDescription: String!
    images: [String!]!
    latitude: Float!
    longitude: Float!
    addressLine1: String!
    addressLine2: String!
    addressCity: String!
    postcode: String!
    zone: Zone!
    tags: [Tag!]!
    contactNumber: String!
    pdfMenus: [Menu!]!
    lists: [List!]!
    listItems: [ListItem!]!
    instagramHandle: String!
    bookingUrl: String!
    bookingType: String!
    wifiSSID: String!
    wifiPassword: String!
    openingHours: OpeningHours!
    published: Boolean!
  }

But, this is the key bit... This exactly what I was trying to avoid. I hate having to define all my types in one big *.graphql file, or even in a multitude of small files dotted around. I much prefer the way that the files are structured in my branch above.

I think that there is some magic going on with importSchema whereby it either doesn't import types that are defined locally, or simply overwrites the imported types with the manually defined ones?

Clearly that won't work with the way the code is structured (importSchema knows nothing about what is defined locally, as it's only merged in later), but there must be a way to "merge" them, keeping the user-defined types over any imported ones?

@divyenduz
Copy link
Contributor

I added a test case for collision in GraphQL import.

That part looks to be working as expected.

@divyenduz
Copy link
Contributor

But I think you want to merge imported type with type defined in JS/TS. Let me get back to you on this.

@Siyfion
Copy link
Author

Siyfion commented Aug 1, 2018

@divyenduz yes, that's precisely my use-case. It's all about how to combine the use of graphql-import with types defined in JS/TS using the gql decorator. The merge happens here, but then there's no conflict-resolution

I appreciate it's not really a graphql-import issue, but as I think I mentioned in our chat on Slack, it would be really good to have a known working solution for this use-case!

@divyenduz
Copy link
Contributor

@Siyfion : Noted, this can happen via some additional tooling exposed by graphql-import or a separate tooling altogether.

I remember that you tried mergeSchemas to resolve this. What was the outcome?

@Siyfion
Copy link
Author

Siyfion commented Aug 1, 2018

If I'm completely honest, I got a little worried when it started trying to merge two different root Query objects, it's quite possible that this is the correct solution to go down, though I fear my lack of knowledge of the internal workings of the GraphQL AST may have scared me off! 😊 As I didn't fully understand what it was doing.

@Siyfion
Copy link
Author

Siyfion commented Aug 3, 2018

@marktani & others

After a lot of discussion & help from @divyenduz, I think I've got somewhere close to a workable solution! In short, if you want to keep your GraphQL in JS and modular, you need to convert it back to SDL, merge it all together, add any # import statements and then run it through graphql-import, like so:

// Convert the typeDefs BACK to SDL from objects
const schemaSDLArray = typeDefs.map(typeDef => print(typeDef))
// Push the imports as a simple string to the top of the array
schemaSDLArray.unshift(imports)
// Merge the array into one big SDL "file"
const schemaSDL = schemaSDLArray.join('\n')
// Run the SDL through importSchema, so that the dependencies are imported
const withImportsSDL = importSchema(schemaSDL)

I've updated my branch to now have the new code in it, with the main commit being: https://github.com/Siyfion/node-graphql-starter/commit/7adaca411161a1e93bf356a8de051d50e2bd163c

However, I have uncovered an issue with graphql-import as a result; all occurrences of extend type ... in the SDL are being removed post-import. :/

@Siyfion
Copy link
Author

Siyfion commented Aug 3, 2018

The issue with extend type ... seems related to #42 however, I think that issue is dealing with a scenario whereby the original type and the extension are in separate files. In this case, both the type and the extension are all in the one SDL block. Type extensions are now part of the spec (http://facebook.github.io/graphql/June2018/#sec-Object-Extensions).

This is now a blocker to getting this issue resolved unfortunately.

@Siyfion
Copy link
Author

Siyfion commented Aug 6, 2018

@divyenduz I actually don’t think that the "support” I’m requiring should be a very big change at all… Perhaps it wasn’t fully explained? I don’t need graphql-import to support extend type X statements in different files, and handle the imports. All I need it to do is not wipe out extend type from the “root” file being imported. eg:

# import Author from "./someImport.graphql"

type Story {
  name: String!
}

extend type Story {
  author: Author!
}

at the moment the extend just gets ignored and removed from the output, if this file is run through graphql-import.

The extend type code is all handled nicely by Apollo Server v2, so I really just need the import to leave that statement alone and not delete it!

@johannpinson
Copy link

hey @Siyfion

I had the same issue and used a more "manual" method to import types that I need:

import * as fs from 'fs'
import * as path from 'path'
import { gql } from 'apollo-server-express'

const prismaDefs = gql(
  fs.readFileSync(
    path.resolve(path.join(__dirname), '../generated/prisma.graphql'),
    'utf8',
  ),
)

export const importPrismaDefs = (namesToImport: string[]) => {
  const definitions: any[] = prismaDefs.definitions
    .filter((definition: any) => {
      if (definition.name) {
        return namesToImport.includes(definition.name.value)
      }

      return false
    })

  return {
    ...prismaDefs,
    definitions,
  }
}

I used this function to import all "inputs" that I didn't want to recreate like UserCreateInput or UserWhereInput (especially with all "where"-style inputs which can have dozens and dozens of fields).
It forced me to list all necessary types (around forty), but still I migrate from Yoga to Apollo-server v2, I was normally able to extend them.

So I use you logic which works well!
But from your differents tests, did you succeed to find a compromise to allow to use extend type?

@Siyfion
Copy link
Author

Siyfion commented Aug 9, 2018

Hey @johannpinson

Thats a pretty neat way to do it! I haven't found a way to use extend with types defined in the "imports". However, I can use extend internally.

How does your strategy cope with "overrides"? I guess that as you are importing things entirely manually, you just don't import them and then redefine them in your schema?

Are you using GraphQL in JS (gql tags) or *.graphql files in your Apollo Server v2 project? It seems as though they (and I) prefer using a GraphQL in JS approach!

@johannpinson
Copy link

@Siyfion

Finally I also use extendlogic by this way:

const baseTypes = gql`
  # Original queries, mutations, types and input that I need for my server
  ...
`

const extendedTypes = gql`
  extend type User {
    test: String
  }
`
-----

import { baseTypes, extendedTypes } from './types'

// Convert the typeDefs BACK to SDL from objects
const schemaSDLArray: string[] = [print(baseTypes)]
// Push the imports as a simple string to the top of the array
schemaSDLArray.unshift(imports)
// Merge the array into one big SDL "file"
const schemaSDL: string = schemaSDLArray.join('\n')
// Run the SDL through importSchema, so that the dependencies are imported
const baseTypeDefs: DocumentNode = gql(importSchema(schemaSDL))
// Merge all differents types
const typeDefs = [baseTypeDefs, extendedTypes]

And it looks to work 😄(and yes I know, the unshift is useless ^^).

Like you see, I use too the apollo-tag (aka gql) since I switched to Apollo-server v2.
It allow me to make some ${custom} code inside the tag 😉
(well, vscode-graghql -- hey @divyenduz 👋 -- doesn't highlight it well, but I'm sure I will need it more and more in the future).

By the way, the extends that I use a really simple for the moment like add a field into a type or an input.
I doesn't have yet a case when it will need to update a more, more than just "add" it.
In fact, it will be also the unextend key which I dream 😆

@Siyfion
Copy link
Author

Siyfion commented Aug 9, 2018

That code looks familiar @johannpinson! 😉

I may well have a play with your approach later on tonight, or early next week. I feel like you might have cracked the final piece of the puzzle! 👍

Do you have a repo with all this as a working example?

@johannpinson
Copy link

Ha ah, sure @Siyfion!
I said before I moved into your logic when I discover it earlier this week 😄

After I not sure it the "perfect" final piece, because we use gql, transform it in string and re-parse it in gql 😕 but for the moment it works.

The only example I have it the project on which I work for a client, so I will try to create a demo asap (because I will be on vacation few days from tomorrow).

@Siyfion
Copy link
Author

Siyfion commented Aug 13, 2018

Hey @johannpinson I've been trying things out with your method, and I've got it all working...

The good news is that your block:

// Convert the typeDefs BACK to SDL from objects
const schemaSDLArray: string[] = [print(baseTypes)]
// Push the imports as a simple string to the top of the array
schemaSDLArray.unshift(imports)
// Merge the array into one big SDL "file"
const schemaSDL: string = schemaSDLArray.join('\n')
// Run the SDL through importSchema, so that the dependencies are imported
const baseTypeDefs: DocumentNode = gql(importSchema(schemaSDL))
// Merge all differents types
const typeDefs = [baseTypeDefs, extendedTypes]

no longer needs to exist! You just need to merge all your arrays of typeDefs into one large array that you pass into the ApolloServer constructor.

@Siyfion
Copy link
Author

Siyfion commented Aug 13, 2018

Latest (working) code now available here:
https://github.com/Siyfion/node-graphql-starter/tree/graphql-import

@johannpinson
Copy link

Hello @Siyfion
Yes the solution that I gave works, but it requires you write any type you need inside the imports.js.
It's why I prefer your version with graphql-import, because the package auto-imports the type/object that you can need without declare all of them.

After it depends how you want to manage the imports of external types :

  • auto-import default and missing type with graphql-import, so no worries about forget one
  • manual import of what you need, which allow you to see each imported object precisely

One perfect solution which be that graphql-import auto-add a comment on types imported and so we will can see this flag inside GraphQL Playground for example 😉

@Siyfion
Copy link
Author

Siyfion commented Aug 13, 2018

Yeah, I prefer the explicit import syntax, as if your GraphQL Playground endpoint is exposed publicly, they don't get to see the internal workings of your entire DB structure, which the automatically imported Prisma types/imports, etc. all give away a bit!

@johannpinson
Copy link

Yes I see but it forces me to have all this import for one type created for example:

const TypeImportedFromPrisma = importPrismaDefs([
  // Top-level requested type by previous declarations
  'SpaceCreateInput',
  'SpaceUpdateInput',
  'SpaceWhereUniqueInput',

  // Inherit requested type
  // For 'SpaceCreateInput'
  'StoryCreateManyWithoutSpaceInput',
  'StoryCreateWithoutSpaceInput',
  'StoryWhereUniqueInput',
  'StoryCreateimagesInput',
  'UserCreateOneWithoutSpacesInput',
  'UserCreateWithoutSpacesInput',
  'UserWhereUniqueInput',
  // For 'SpaceUpdateInput'
  'StoryUpdateManyWithoutSpaceInput',
  'StoryUpdateWithWhereUniqueWithoutSpaceInput',
  'StoryUpsertWithWhereUniqueWithoutSpaceInput',
  'StoryUpdateWithoutSpaceDataInput',
  'StoryUpdateimagesInput',
  'UserUpdateOneWithoutSpacesInput',
  'UserUpdateWithoutSpacesDataInput',
  'UserUpsertWithoutSpacesInput',
])

and my final model will have around a twenty type which can have relation between them 😆

@Siyfion
Copy link
Author

Siyfion commented Aug 13, 2018

True, I could argue that exposing create/update/upsert input types is very dangerous if you have any kind of permissions model, but I'm guessing you know that!?

We don't expose any Prisma types for any mutations. They don't allow us to give the level of control that we needed.

@johannpinson
Copy link

Yes all mutations (and some queries/fields) will be cover by graphql-shield + graphql-middleware for custom checks.
I'm testing also the Schema directives as solution, but the first permission layer will be implemented in front.
So to simulate the JWT Auth + context of request will be a good challenge to force the security ^^

On your side, you will "rewrite" all input and remove "connect"-like field available from Prisma?

@divyenduz divyenduz removed their assignment Jan 4, 2019
@vadistic
Copy link

Few months ago I got so frustrated with exactly all this, that I made a tool. Weird that I did not notice this issue before.

Frankly - right now I'm more partial to interpolated string literals in ts, but I'm plugging this as some curiosity: https://github.com/vadistic/graphql-override

It even integrates with prisma config (and works)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants