Feature/fix circular refs

[chrisdrackett]

merge simple circular example with @godness84's circular ref fix.

[chrisdrackett]

@godness84 I updated this branch with your code. I'm still getting issues in RootStore.base.ts around:

'RootStoreBase' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.

I think this is the same issue I'm seeing on the toggle action in TodoModel.ts

[godness84]

@chrisdrackett I pushed a new fix to my branch!

I applied the same anti-circular-refs technique onto RootStore. Your example (6-typegen) was raising that any issue on RootStore because whenever a model referenced by RootStore (Todo model in your case) defines an action whose return type depends on the return type of an action of the RootStore, it creates a new circular ref issue.

With that fix I pushed, it seems solved. 🤞

[chrisdrackett]

@godness84 ok, this is awesome!

• I still wish we had a way to get around needing the as() bit, but my gut is its still worth shipping this and then keeping an issue open around improving that.
• We'll need to make sure to get the tests working before we merge this.
• This change brings up a point I think I've made in the past, but we're often changing how XXXModel.ts files are generated which requires some level of migration for users who are using the old format. As we get closer to a 1.0 it might be good to have a strategy around communicating these updates and/or providing a codemod or similar.

[beepsoft]

It seems to work all right with my schema as well, awesome!

The "self" casting is a bit strange but I can live with that for now. Although in this form it could be made compatible with future implementations:

/**
 * someModel
 */
export const someModel = someModelBase
  .actions(_self => {
    let self = as(_self);
    
    // This is an auto-generated example action.
    function log() {
      console.log(JSON.stringify(self))
    }
    
    return {
      log
    }
  })

Formulated this way the actions will always use "self" and if the implementation changes and we don't need as() then as() will become a noop (and further generation runs won't even generate it).

But of course it is more typing and error prone to have a function and then return the object literal containing the func than just having an object literal as the function's result.

[godness84]

About the as() thing, consider that:

• it's not strictly required. If you just type self, typescript understands its type, but without the references to other models. In this sense, it would have been better to name it withRefs().
• if you use it, as an added bonus, you can forget the order in which actions/views/props are declared since you don't incur in the problem described here. In this sense, I named it as().

[godness84]

@chrisdrackett I updated my branch with a fix to pass tests! 😉

[chrisdrackett]

@beepsoft the only downside to your recommendation (as I understand it) is that it will give you a performance penalty as noted at the bottom of https://github.com/mobxjs/mobx-state-tree#using-a-mst-type-at-design-time

[chrisdrackett]

@godness84 this looks good to me! I'd love at least one more person to review before we merge. Maybe @beepsoft or @zenflow can take a last look?

[beepsoft]

@chrisdrackett Regarding the performance penalty issue, you are probably right.

The PR seems to be OK for me, tests run all right and I've been using the models generated by feature/fixCircularRefs for a week or so now with no problem.

[godness84]

@chrisdrackett @beepsoft is the performance penalty issue related to the use of the as() wrapper?

[beepsoft]

@godness84 No, as() is purely TS and compile time thing, the performance issue is with this 2 step approach I sketched to have a "clean" self in actions:

/**
 * someModel
 */
export const someModel = someModelBase
  .actions(_self => {
    let self = as(_self);
    
    // This is an auto-generated example action.
    function log() {
      console.log(JSON.stringify(self))
    }
    
    return {
      log
    }
  })

The MST docs say:

As a last resort, although not recommended due to the performance penalty (see the note below), you may declare the views in two steps:

const Example = types
  .model('Example', { prop: types.string })
  .views(self => {
      const views = {
        get upperProp(): string {
            return self.prop.toUpperCase();
        },
        get twiceUpperProp(): string {
            return views.upperProp + views.upperProp;
        }
      }
      return views
  }))

NOTE: the last approach will incur runtime performance penalty as accessing such computed values (e.g. inside render() method of an observed component) always leads to full recompute (see this issue for details). For a heavily used computed properties it's recommended to use one of above approaches.

[godness84]

@beepsoft Mmm, I have some difficulties to understand 🤔 , maybe I'm missing something. What is your as() wrapper doing in the example you wrote? Isn't it the same as() wrapper I proposed? In this case, you're casting self once, and then using that casted self everywhere instead of casting it every time you use it. Am I wrong?

[beepsoft]

@godness84 Yes, it is the same as(). I was just thinking what we could have in the generated model code so that in case in the future we can get rid of as() we would not need to update the actions, which would be full of as(self)'s. So, what I tried to achieve is to be able to use always self in the actions.

[godness84]

@beepsoft Ok, understood. But if your example code suffers from performance penalties, even my code should suffer from it - they're very similar, you just cast self once and reuse it everywhere.
Anyway, I think neither of them will incur in performance penalties: in the linked issue (mobxjs/mobx-state-tree#818 (comment)) it's written that the problem is that the views variable is not observable, but in our case self is observable.

Hope it's understandable what I wrote 😄

P.S. I tried to reproduce the issue described in the MST docs, but I failed: I could not see the [mobx.trace] 'PROP NAME' is being read outside a reactive context. Doing a full recompute message when using trace(). Maybe the performance issue is not an issue anymore?

[chrisdrackett]

@godness84 I went ahead and updated the example projects using this code. I'm running into issues in example 3 that I think are worth looking at before we merge. I'll note them inline now.

[chrisdrackett]

I'm getting Property 'likes' does not exist on type 'MessageModelType'. here. Should this not work when using as()?

[chrisdrackett]

same as above here.

[chrisdrackett]

both sortedMessages and message are coming through as any for me.

[chrisdrackett]

for some reason all the self's on rootStore are giving me a Parameter 'self' implicitly has an 'any' type, but a better type may be inferred from usage.

[beepsoft]

I'm not sure if actually related to this PR, but I just found that in a Model's action, when I want to access the mutate() or query() of the store, it doesn't show up in autocomplete.

I have something like this:

export const choicesModel = choicesModelBase
  .actions(self => ({
    doSmething() {
      self.store.[here I expect to see mutate(), query(), etc. listed when I press control+space]
    }
  }))

Should this work?

[godness84]

@chrisdrackett @beepsoft Uh oh, it looks like I fell down the rabbit hole of circular refs, but maybe I reached the top again. Maybe.

The 3-twitter-clone example has 2 different kinds of problems:

1. the return type of some views is not explicit and depends on the result of other model's actions (e.g. RootStore::me(), MessageModel::isLikedByMe()
2. a new circular ref caused by RootStore that stores a ref to MessageModel (sortedMessages)

To solve 1) it is enough to make explicit the result type.
To solve 2) we must break the circularity between MessageModel and RootStore, and it can be done on one of the two sides. I prepared two different repos, each one solves it on one side:

• https://github.com/godness84/mst-gql/tree/feature/fixCircularRefs-v1
• https://github.com/godness84/mst-gql/tree/feature/fixCircularRefs-v2

While working on this, a different way of solving circular refs came to my mind. So far we have solved it by removing ref props from the type of the model and reintroducing them on the type of the model instance. But it caused us to use the as() wrapper and to split the definition of the base models into two parts: one without refs and the other with refs.

I propose a different way: use type assertion directly on the model type (IModelType) instead of doing it on the model instance type. I cannot express myself better, the code is better than a thousand words: in this branch (https://github.com/godness84/mst-gql/tree/feature/fixCircularRefs-safe) you can find a proof of concept that works quite well.

How it works (few and ugly words):

• every ref of the model is typed as any (e.g. todos: types.array(types.late((): any => TodoModel)). In this way we break the circularity.
• safeRefs helper method does type assertion directly on the model type, giving the correct type to the refs.
• safeRefs is not generated every time (as it was for the as() wrapper before) but it's inside the mst-gql package. In this way it's possibile to reuse it easily when your models introduce new circular refs (as it happens in the 3-twitter-clone example)

I know it means we should start over, with a new PR, new updated examples and so on, but I think it's worth it! Let me know :)

[beepsoft]

@godness84 I have no problem having a better solution without as(). :-) Although for me things seemed to work with your previous solution as well.

Now I tried your https://github.com/godness84/mst-gql/tree/feature/fixCircularRefs-safe branch and for me the generated models and all the selfs are all of type any at the moment. I've built mst-gql from your branch and npm linked to my project and checked that I have safeRefs() in mst-gql and also in the generated models.

So, what should we expect to have working in feature/fixCircularRefs-safe?

[godness84]

Mmm, weird. TS version? Do you a repo that I could try with? Il gio 31 ott 2019, 21:47 beep <notifications@github.com> ha scritto:

…

@godness84 <https://github.com/godness84> I have no problem having a better solution without as(). :-) Although for me things seemed to work with your previous solution as well. Now I tried your https://github.com/godness84/mst-gql/tree/feature/fixCircularRefs-safe branch and for me the generated models and all the selfs are all of type any at the moment. I've built mst-gql from your branch and npm linked to my project and checked that I have safeRefs() in mst-gql and also in the generated models. So, what should we expect to have working in feature/fixCircularRefs-safe? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#128?email_source=notifications&email_token=AA5NBDOP2MZQY3BTMKEM45LQRM75PA5CNFSM4JCKXUX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECZGIFY#issuecomment-548561943>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA5NBDO63V54WGEEDJAKHRLQRM75PANCNFSM4JCKXUXQ> .

[beepsoft]

TS 3.6.4 ("typescript": "^3.6.4").

I just did the build/npm link and run a clean generation like

node generator/mst-gql-scaffold.js --format ts --outDir ....polls/src/models-polls-fixCircularRefs-safe  ....polls/schema.graphql

with this schema:

https://gist.github.com/beepsoft/84a64102a80a3ac7eb1f656b64fc2af8

Then checked the generated sources in my IDE and saw any in every model.

[godness84]

@beepsoft I tried your schema and it works for me. Sure that safeRefs is found in the package and that it is written in the generated files?

[godness84]

@beepsoft what happens if you try to scaffold the example projects in mst-gql repo?

[beepsoft]

If I do the scaffolding in the mst-gql project, then in eg. src/models-polls-fixCircularRefs-safe/RootStore.ts I see

Error:(6, 74) TS2307: Cannot find module 'mst-gql'.

As a result this import line won't actually load safeRefs and the others.

import { MSTGQLStore, configureStoreMixin, QueryOptions, safeRefs } from "mst-gql"

[godness84]

It means you have problems in linking or something. Did you launch yarn command in the root folder and in the example folder?

[beepsoft]

@godness84 Ok, now I generated the model into the 1-getting-started example so that the mst-gql module reference would be correct.

Now, with this schema

https://gist.github.com/beepsoft/84a64102a80a3ac7eb1f656b64fc2af8

autocomplete seems to work (in VSCode better than in IntelliJ).

One thing I found is that I had to make the type of the forEach parameter explicitly v: choicesModelType so that I could access v.poll.created_at. Otherwise v would be just any.

/**
 * choicesModel
 */
export const choicesModel = choicesModelBase
  .actions(self => ({
    foo() {
      self.poll.choices!.forEach((v: choicesModelType) => v.poll.created_at);
    }
  }))

[chrisdrackett]

@godness84 @beepsoft sorry for the delay in chiming in on this, I've been pretty busy with other things!

I haven't yet had time to try the new repo myself but it sounds really promising!

@godness84 if you want to create a new PR here we can move discussion related to that work to its PR?

[RXminuS]

I'll double check on monday that this indeed fixes #117. Great job @godness84 !