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

Recursive type maximum call stack exceeded #11

Closed
Amareis opened this issue Oct 4, 2018 · 11 comments
Closed

Recursive type maximum call stack exceeded #11

Amareis opened this issue Oct 4, 2018 · 11 comments

Comments

@Amareis
Copy link

Amareis commented Oct 4, 2018

This code (in pure mst) works well:

const LateMessage = types.late((): IAnyModelType => Message)

export const Message = types.model('Message', {
    id: types.identifier,
    quoted: types.maybeNull(LateMessage),
})

But this code is failing (when creating from snapshot):

const LateMessage = types.late((): IAnyModelType => Message)

const MessageData = types.model('MessageData', {
    id: types.identifier,
    quoted: types.maybeNull(LateMessage),
})

class MessageCode extends shim(MessageData) {
}

export const Message = mst(MessageCode, MessageData, 'Message')

image

@Amareis
Copy link
Author

Amareis commented Oct 7, 2018

@jjrv can you fix it or explain how it can be fixed in classy-mst, please?

@jjrv
Copy link
Member

jjrv commented Oct 9, 2018

It looks like a recursive type: Message contains a field quoted containing another Message and as far as the types are concerned the nesting could be infinite, even if in practice you only nest once. The section Recursive types details how to get that working.

@Amareis
Copy link
Author

Amareis commented Oct 9, 2018

But with types.late typescript don't complain about circular type. There is only runtime error because of incorrect handling of maybeNull, I think.

@jjrv
Copy link
Member

jjrv commented Oct 9, 2018

I found that a slightly improved fix to #8 also would fix this, however while disabling polymorphism support for the Message type (so you cannot then inherit from it, serialize to a snapshot and correctly deserialize different types of messages).

The diff of classy-mst.ts is:

-       let Model = Data.preProcessSnapshot(
-               // Instantiating a union of models requires a snapshot.
-               (snap: any) => snap || {}
-       ).postProcessSnapshot((snap: any) => {
+       let Model = Data.postProcessSnapshot((snap: any) => {

-       return(polymorphic(Code, Model, modelName));
+       return(Model as any);

Can you test it? I'll need to add a new options object to allow toggling this fix on/off.

@jjrv
Copy link
Member

jjrv commented Oct 9, 2018

Actually it seems a mobx-state-tree bug has been fixed and that preProcessSnapshot hook is no longer needed. Meanwhile it actually triggers this bug... For some reason simply removing the hook fixes this, and as you see the hook does nothing but ensures there is a snapshot, because without one creating union instances used to fail.

@Amareis
Copy link
Author

Amareis commented Oct 9, 2018

Sooo, what is the final fix? Just remove preProcessSnapshot form classy-mst.ts?

@jjrv
Copy link
Member

jjrv commented Oct 9, 2018

Try and see if it works for you. Seems to still cause some tests to fail. I'll fix this one way or another and release a new version. Disabling polymorphism for the type may still be necessary.

@Amareis
Copy link
Author

Amareis commented Oct 9, 2018

Ok, i'll try it.

@Amareis
Copy link
Author

Amareis commented Oct 9, 2018

Yeah, removing of preProcessSnapshot did the trick!

Anyway, I already refactor my Message type to use types.reference to quoted, so it working without fix, but thank you for help, I'll wait for the release.

@jjrv
Copy link
Member

jjrv commented Oct 9, 2018

OK, version 3.5.0 is out. It now works with a flag:

export const Message = mst(MessageCode, MessageData, 'Message', { sealed: true })

@jjrv jjrv closed this as completed Oct 9, 2018
@jjrv
Copy link
Member

jjrv commented Oct 9, 2018

Thanks for reporting!

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

2 participants