Issues with typescript generated models

krstns commented 2 years ago

Hello

I have a few issues with generated models by mst-gql 0.15.0. My project fails to compile.

All of the exported types cause circular reference issues. I have to replace: export type RootStoreType = Instance<typeof RootStore.Type>; with export interface RootStoreType extends Instance<typeof RootStore.Type> {} and similar.
TS7022: 'RootStore' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer. (this one is also fixed by changes made in 1.)
TS2742: The inferred type of 'RootStore' cannot be named without a reference to 'mst-gql/node_modules/graphql'. This is likely not portable. A type annotation is necessary.
Incorrect handling of optional type args in the model selector query. One of my fields is generated like this: instances(builder: string | ActionConnectionModelSelector | ((selector: ActionConnectionModelSelector) => ActionConnectionModelSelector) | undefined, args?: { first?: number, last?: number, before?: string, after?: string }) { return this.__child(instances(first: ${JSON.stringify(args['first'])}, last: ${JSON.stringify(args['last'])}, before: ${JSON.stringify(args['before'])}, after: ${JSON.stringify(args['after'])}), ActionConnectionModelSelector, builder) }

Inside the JSON.stringify calls, the args can be undefined.

beepsoft commented 2 years ago

Can you maybe share a minimal project where these errors are produced?

All of the exported types cause circular reference issues. I have to replace: export type RootStoreType = Instance<typeof RootStore.Type>; with export interface RootStoreType extends Instance<typeof RootStore.Type> {} and similar.

I haven't had problems with this, but I think there should be some updates here, because Instance<typeof RootStore.Type> is deprecated and Instance<typeof RootStore> should be used instead.

TS2742: The inferred type of 'RootStore' cannot be named without a reference to 'mst-gql/node_modules/graphql'. This is likely not portable. A type annotation is necessary.

I also had this for an old project where I just wanted to update mst-gql. This helped:

  "resolutions": {
    "graphql": "14.3.0"
  }

For newly created projects and adding the necessary mst-gql dependencies this error didn't come up.

4. Incorrect handling of optional type args in the model selector query. One of my fields is generated like this: instances(builder: string | ActionConnectionModelSelector | ((selector: ActionConnectionModelSelector) => ActionConnectionModelSelector) | undefined, args?: { first?: number, last?: number, before?: string, after?: string }) { return this.__child(instances(first: ${JSON.stringify(args['first'])}, last: ${JSON.stringify(args['last'])}, before: ${JSON.stringify(args['before'])}, after: ${JSON.stringify(args['after'])}), ActionConnectionModelSelector, builder) }

Hopefully I have a fix for this one: https://github.com/mobxjs/mst-gql/pull/361

You may give it try using:

yarn add 'beepsoft/mst-gql#mst-gql-v0.15.0-gitpkg'

@jesse-savary, can you please take a look at my PR-s #361, #363?

krstns commented 2 years ago

Hi, I will give you a report soon. I's funny but we have removed the 'instance' property from our APIs and suddenly:

generated models use interfaces instead of types (???)
the issue with the `instance' field is gone (obviously).

I will revert to the previous schema and will make the APIs publicly available (it's a PoC anyway) so you will be able to test it yourself.

krstns commented 2 years ago

I came back to see if there are any answers and I have noticed my previous comment is gone... I have included two schema files with examples... I think I must have messed up something. Here are the schema files with and without the 'instances' properties schema_without_instances.txt

schema_with_instances.graphql.txt

EDIT: We've NOT been using your version to generate models, but old 0.14.x ... That's why i didn't see the issue with args. I've tried your custom version and it still produces the error.

krstns commented 2 years ago

@beepsoft i've edited my previous comment because of a mistake in my tests, but just to ping you, the version 'beepsoft/mst-gql#mst-gql-v0.15.0-gitpkg' does not solve the issue with args.

beepsoft commented 2 years ago

What is the error you are getting with beepsoft/mst-gql#mst-gql-v0.15.0-gitpkg? And can you maybe share your graphql schema?

krstns commented 2 years ago

I've shared the two schemas here: https://github.com/mobxjs/mst-gql/issues/364#issuecomment-1115008552 Could you give me your email so I can share the schema privately?

beepsoft commented 2 years ago

That's too bad there's no private messaging in GH and I don't feel comfortable sharing email addresses here. Instead, try to PM me on twitter @ beepshow.

krstns commented 2 years ago

my thought exactly... I've spent a few minutes looking for a DM. Thanks.

beepsoft commented 2 years ago

I just created a new CRA project

npx create-react-app my-app --template typescript
cd my-app
yarn add mobx mobx-state-tree mobx-react react react-dom 'beepsoft/mst-gql#mst-gql-v0.15.0-gitpkg' graphql-request
yarn add graphql graphql-tag

and added these scripts:

    "gql:gen": "mst-gql --format ts --outDir src/models schema_without_instances.graphql",
    "gql:build": "tsc --extendedDiagnostics"

This is my final package.json:

{
  "name": "my-app",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "@testing-library/jest-dom": "^5.16.4",
    "@testing-library/react": "^13.2.0",
    "@testing-library/user-event": "^13.5.0",
    "@types/jest": "^27.5.0",
    "@types/node": "^16.11.33",
    "@types/react": "^18.0.8",
    "@types/react-dom": "^18.0.3",
    "graphql": "^16.4.0",
    "graphql-request": "^4.2.0",
    "graphql-tag": "^2.12.6",
    "mobx": "^6.5.0",
    "mobx-react": "^7.3.0",
    "mobx-state-tree": "^5.1.4",
    "mst-gql": "beepsoft/mst-gql#mst-gql-v0.15.0-gitpkg",
    "react": "^18.1.0",
    "react-dom": "^18.1.0",
    "react-scripts": "5.0.1",
    "typescript": "^4.6.4",
    "web-vitals": "^2.1.4"
  },
  "scripts": {
    "start": "react-scripts start",
    "build": "react-scripts build",
    "test": "react-scripts test",
    "eject": "react-scripts eject",
    "gql:gen": "mst-gql --format ts --outDir src/models schema_without_instances.graphql",
    "gql:build": "tsc --extendedDiagnostics"
  },
  "eslintConfig": {
    "extends": [
      "react-app",
      "react-app/jest"
    ]
  },
  "browserslist": {
    "production": [
      ">0.2%",
      "not dead",
      "not op_mini all"
    ],
    "development": [
      "last 1 chrome version",
      "last 1 firefox version",
      "last 1 safari version"
    ]
  }
}

Then generated and tsc compiled the generated sources like this:

yarn gql:gen
yarn gql:build

This is my result:

yarn run v1.19.2
$ tsc --extendedDiagnostics
Files:                         915
Lines of Library:            27579
Lines of Definitions:        91450
Lines of TypeScript:         19803
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Nodes of Library:           118973
Nodes of Definitions:       291688
Nodes of TypeScript:        101065
Nodes of JavaScript:             0
Nodes of JSON:                   0
Nodes of Other:                  0
Identifiers:                172847
Symbols:                    191839
Types:                       67593
Instantiations:             359888
Memory used:               296804K
Assignability cache size:    34212
Identity cache size:           911
Subtype cache size:            808
Strict subtype cache size:  309426
I/O Read time:               0.20s
Parse time:                  1.85s
ResolveModule time:          0.35s
ResolveTypeReference time:   0.04s
Program time:                2.71s
Bind time:                   0.87s
Check time:                  9.56s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                 13.15s
✨  Done in 13.57s.

I tried it with both schema_without_instances.txt and schema_with_instances.graphql.txt and both of them generated and compiled all right (needed to rename them to .graphql).

It might be better if you shared a (failing) project on github, so I could see how to reproduce the error.

krstns commented 2 years ago

I've tried to reach out to you on Twitter but you have your DMs blocked. I will try to generate it again.

beepsoft commented 2 years ago

I've tried to reach out to you on Twitter but you have your DMs blocked. I will try to generate it again.

Oops, you are right, now I enabled DM-s.

brkastner commented 2 years ago

I haven't looked into how to fix this within the library itself but the manual workaround working locally for me (for a related field called events) is to replace this:

return this.__child(`events(distinctOn: ....

with this:

return this.__child(args == null ? 'events' : `events(distinctOn: ....

so that it falls back to the old functionality when args is not provided.

Another issue to note is that some of the required dependencies used by the omitted part of the above code are not properly imported automatically.

beepsoft commented 2 years ago

Hopefully this together with some other issues affecting v0.15.0 is already fixed with the PR-s merged into main. @jesse-savary could you maybe create a beta release on NPM so that we can try the new version in our projects?

krstns commented 2 years ago

It took me a little, but here's a repo showing my issues. I've used the last commit + a super simple graphql example (attached). The generated TS code is unusable. I hope I didn't mess up the import but well.

https://github.com/krstns/mst-gql-issues

yarn install
yarn ng build

mobxjs / mst-gql

Issues with typescript generated models #364