Circular dependencies between models about mst-gql HOT 48 CLOSED

So I'm thinking we can handle this by manually generating the TS types. TS shouldn't have issues with the circular dependencies in general. For example, the following code compiles fine:

interface Post {
  id: string
  comment?: Comment
}

interface Comment {
  id: string
  post: Post
}

const post: Post = {
  id: 'a',
  comment: {
    id: 'b',
    post: {
      id: 'c',
    }
  }
}

Having TS figure out the types from the model itself is nice usually, but as we're generating this code in any case, we may as well generate the TS types too and get around this circular any issue.

@mweststrate I know you've dealt with circular dependencies quite a lot. What are your thoughts on this?

from mst-gql.

+1 i have circular deps in gql

base file generate at base model field

import { UserGroupModel } from "./UserGroupModel"

export const UserGroupModelBase = MSTGQLObject
  .named('UserGroup')
  .props({
 //
    manageableUserGroups: types.optional(types.array(MSTGQLRef(types.late(() => UserGroupModel))), []),
//
  })`

at the same time custom model required import UserGroupModelBase

import { UserGroupModelBase } from './UserGroupModel.base';

export const UserGroupModel = UserGroupModelBase.actions(self => {
  return {
    addRoleId(id) {
///

as a result -> circular deps + build error
Cannot read property 'actions' of undefined at UserGroupModel

from mst-gql.

🎉 Great work @godness84, thanks for your solution and @chrisdrackett for managing the merge! I'm so happy with this issue being solved!

from mst-gql.

#69 should already make this much better

from mst-gql.

https://github.com/pahen/madge can be helpful in find and resoving circular deps. My current models have 36 of it.

from mst-gql.

there seems to be something going on with TS versions past 3.4 that make things like this project and others I use (graphql-nexus/nexus-prisma#291) have issues. I haven't taken the time to dig into it (for now I'm just locked to 3.4.5).

from mst-gql.

I went ahead and put together a draft branch for this work here: #110

This was a quick first pass and certainly missing a lot. It also currently does not do any generation.

Hopefully this can act as a discussion starter!

from mst-gql.

@beepsoft I've started a new PR that's a bit better #117 . The issue that it solves is that in certain cases the actual import is not yet defined depending on the loading order. By using the internal.js module you get more control over the exact loading order. However, it doesn't solve anything about the circular type declarations preventing proper type inference on models that reference themselves.

from mst-gql.

I just tried if it is any better with TS 3.7.0-beta, but unfortunately it is not.

from mst-gql.

(@onomated wrong @ in previous message, sorry :-) )

I've never seen such webpack error, sorry, then it is a different issue.

from mst-gql.

+1, in my (complicated enough) domain models generated models are useless because of circular dependencies.

from mst-gql.

Against the comment in https://github.com/mobxjs/mst-gql/blob/master/generator/generate.js#L354, types.late isn't enough to prevent circular dependencies :)

from mst-gql.

@chrisdrackett does editing the generated file solve your problems? (I realise this isn't a long term fix).

@Amareis is the problem with TS types or a JS issue?

from mst-gql.

@elie222 just TS typings. For correct circular references handling we need to use types.late((): IAnyModelType => SomeModel), but if we do it everywhere, all references will be any type. So, this should be applied only at some points, to break the wheel, but currently IDK how to select that points algoritmically.

from mst-gql.

@elie222 editing the generated file does fix the problem, but like @Amareis mentioned because this makes the return type any you only want to do it where needed and not on everything. I'm not sure if that is something the library could detect, but if so, it would be awesome.

I can try and add a note about this if needed to the docs. Its really more a MST thing than a mst-gql thing, but might be worth letting users know about regardless.

from mst-gql.

Yep, that's will really cool! Besides of circular deps handling, it'll make nice typenames in compiler errors, instead of usual mst types garbage.

from mst-gql.

from mst-gql.

Regardless on whether the above is implemented or not, in general two tips that help with breaking circular type dependencies:

1. Break circles by not relying on type inference for method arguments / return types (example: mobxjs/mobx-state-tree#1341 (comment))
2. Use interfaces that extends types (example: https://github.com/mobxjs/mobx-state-tree#using-a-mst-type-at-design-time, the second code snippet)

Hope that helps in the mean time!

from mst-gql.

@elie222 have you done any work on this? I was thinking of giving it a try tomorrow and/or over the weekend

from mst-gql.

from mst-gql.

So I'm thinking we can handle this by manually generating the TS types.

Wouldn't it be possible to use graphql-codegen (https://graphql-code-generator.com) for this? It can generate TS interfaces for the same graphql schema we use in mst-gql-scaffold.js.

from mst-gql.

from mst-gql.

@elie222 I see. And how should this MST compatible TS interface look like? Do you have any idea of this?

from mst-gql.

from mst-gql.

Thanks for these details!

Another idea/question: implementing this in the generator as a general mechanism for any late() type:

mobxjs/mobx-state-tree#943 (comment)

We would then always have the actual prop as of type any (because of IAnyModelType) and a matching view, which returns the prop casted using Instance<...> and would use this view function in code where we want hints from the IDE.

This is still hackish but could be an optional workaround until manually generated TS types happen.

from mst-gql.

I implemented this solution above. It works nicely with TS 3.4, but doesn't work with newer versions.

from mst-gql.

Is there anyone working on @elie222's proposal about generating TS types "manually"?

So I'm thinking we can handle this by manually generating the TS types. TS shouldn't have issues with the circular dependencies in general. For example, the following code compiles fine:

interface Post {
  id: string
  comment?: Comment
}

interface Comment {
  id: string
  post: Post
}

const post: Post = {
  id: 'a',
  comment: {
    id: 'b',
    post: {
      id: 'c',
    }
  }
}

Having TS figure out the types from the model itself is nice usually, but as we're generating this code in any case, we may as well generate the TS types too and get around this circular any issue.

@mweststrate I know you've dealt with circular dependencies quite a lot. What are your thoughts on this?

from mst-gql.

from mst-gql.

I may try to implement it, but I don't know what actually needs to be done here.

Can you give me some hints how it would work? How these generated TS types would be associated with the MSTGQLObject based MST models the generator generates? If that's how it is planned to work at all - I'm a little bit lost here.

from mst-gql.

I think after the types are generated we would just associate them with the return of RootStore.create, for example here: https://github.com/mobxjs/mst-gql/blob/master/examples/1-getting-started/src/app/index.tsx#L11

I think also ideally the user should be able to easily extend these types in case they are adding props, views, actions, etc. to a store themselves.

from mst-gql.

@beepsoft would it be helpful for me to attempt to put together example output for some of the examples in this repo? That might give us a target to discuss before we worry about how to get from the input to output.

from mst-gql.

@chrisdrackett Yes, an actual example would be a great starting point, thank you!

from mst-gql.

I'm a bit swamped to have time to help out so instead I left a $50 bounty on bountysource :-)

from mst-gql.

@beepsoft ok, I'll work on it some today. Not sure if I'll get anything to share as I probably need to learn a lot more about how MST types work in general :D

from mst-gql.

#112 is probably somewhat related

from mst-gql.

@RXminuS does #112 solve the circular dependencies problem? Or which part of it?

from mst-gql.

and this is QL schema

from mst-gql.

@gadzillllla Can you try with #117 and reorder the internal file to see if you can make the module load sooner?

from mst-gql.

@RXminuS we tried it, and it works fine for now

from mst-gql.

@RXminuS had the same issue as @gadzillllla, JS codebase with circular dependencies. Tried running your fork and got this error while generating the gql models

  - [OBJECT] __InputValue
  - [OBJECT] __EnumValue
  - [OBJECT] __Directive
  - [ENUM] __DirectiveLocation
/app/node_modules/mst-gql/generator/mst-gql-scaffold.js:115
    moduleLoadingOrder
    ^

ReferenceError: moduleLoadingOrder is not defined
    at main (/app/node_modules/mst-gql/generator/mst-gql-scaffold.js:115:5)
    at Object.<anonymous> (/app/node_modules/mst-gql/generator/mst-gql-scaffold.js:121:1)
    at Module._compile (internal/modules/cjs/loader.js:936:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:947:10)
    at Module.load (internal/modules/cjs/loader.js:790:32)
    at Function.Module._load (internal/modules/cjs/loader.js:703:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:999:10)
    at internal/main/run_main_module.js:17:11
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

mst-gql.config.js

module.exports = {
  force: true,
  format: "js",
  input: "app/graphql/schema.graphql",
  outDir: "app/frontend/models",
  roots: [] // auto-detect
};

from mst-gql.

@RXminuS this PR seems to provide a solution:

#128

Will be hopefully merged into master soon.

from mst-gql.

#128 is generating TS for JS spec:

export const RootStoreBase: typeof RootStoreBaseNoRefs = RootStoreBaseNoRefs

Also exhibiting the same webpack circular dependencies error while loading app:

fromRequireContextWithGlobalFallback.js:21 Error: Cannot find module 'mst-gql'
    at webpackMissingModule (reactUtils.js:1)
    at Module../app/frontend/models/reactUtils.js (reactUtils.js:1)
    at __webpack_require__ (bootstrap:19)

from mst-gql.

fixed in 0.7.0

from mst-gql.

Thanks to you guys!

from mst-gql.

still facing issue with circular deps

Circular dependency detected: mobxtree/UsersPermissionsUserConnectionUpdatedByModel.base.ts -> mobxtree/UsersPermissionsUserConnectionModel.ts -> mobxtree/UsersPermissionsUserConnectionModel.base.ts -> mobxtree/UsersPermissionsUserGroupByModel.ts -> mobxtree/UsersPermissionsUserGroupByModel.base.ts -> mobxtree/UsersPermissionsUserConnectionUpdatedByModel.ts -> mobxtree/UsersPermissionsUserConnectionUpdatedByModel.base.ts

using TS 3.7.3 and mst-gql 0.12.0

from mst-gql.

@michaelspeed can you give more details? Does it compile? Does TS understand the types when you put the cursor on? Have you customized the code of any model? Maybe you introduced new circular deps?

Just looking at your few lines it's quite impossible to have any clues

from mst-gql.

Here is the generated folder -
mobxtree.zip

and the schema -
schema.graphql.zip

from mst-gql.

The error output looks like this -
error - ReferenceError: Cannot access 'UploadFileConnectionAlternativeTextModelBase' before initialization at Module.eval [as UploadFileConnectionAlternativeTextModelBase] (webpack-internal:///./mobxtree/UploadFileConnectionAlternativeTextModel.base.ts:2:136)

using Next js

from mst-gql.