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

Issues with typescript generated models #364

Open
krstns opened this issue Apr 22, 2022 · 14 comments
Open

Issues with typescript generated models #364

krstns opened this issue Apr 22, 2022 · 14 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@krstns
Copy link

krstns commented Apr 22, 2022

Hello

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

  1. 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.

  2. 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.)

  3. 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.

  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) }

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

@beepsoft
Copy link
Collaborator

beepsoft commented Apr 22, 2022

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

  1. 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.

  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.

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: #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
Copy link
Author

krstns commented Apr 24, 2022

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
Copy link
Author

krstns commented May 2, 2022

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
Copy link
Author

krstns commented May 4, 2022

@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
Copy link
Collaborator

beepsoft commented May 4, 2022

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
Copy link
Author

krstns commented May 4, 2022

I've shared the two schemas here: #364 (comment)
Could you give me your email so I can share the schema privately?

@beepsoft
Copy link
Collaborator

beepsoft commented May 4, 2022

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
Copy link
Author

krstns commented May 4, 2022

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

@beepsoft
Copy link
Collaborator

beepsoft commented May 4, 2022

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
Copy link
Author

krstns commented May 4, 2022

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

@beepsoft
Copy link
Collaborator

beepsoft commented May 4, 2022

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
Copy link

brkastner commented May 9, 2022

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
Copy link
Collaborator

beepsoft commented May 9, 2022

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
Copy link
Author

krstns commented May 24, 2022

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

@jesse-savary jesse-savary added the bug Something isn't working label Nov 6, 2022
@jesse-savary jesse-savary self-assigned this Nov 6, 2022
@jesse-savary jesse-savary added this to Needs triage in Stability and Automated Testing via automation Nov 6, 2022
@jesse-savary jesse-savary added this to the v0.18.0 milestone Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

No branches or pull requests

4 participants