mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.94k stars 640 forks source link

fail is not defined exception instead of assertion #1989

Closed Sinled closed 10 months ago

Sinled commented 1 year ago

Bug report

Describe the expected behaviour https://github.com/mobxjs/mobx-state-tree/blob/master/packages/mobx-state-tree/src/core/node/BaseNode.ts#L166

it seems there is missing fail import, instead "assertion failed: cannot finalise the creation of a node that is already dead" i receive fail is not defined error in my application

Describe the observed behaviour Correct assertion should be thrown

coolsoftwaretyler commented 1 year ago

Hey @Sinled - do you have a minimum reproduction of the error that you're seeing here?

Sinled commented 1 year ago

@coolsoftwaretyler It was 4 months ago, i can't remember exact reproduction.

But if you open this file https://github.com/mobxjs/mobx-state-tree/blob/master/packages/mobx-state-tree/src/core/node/BaseNode.ts#L166

fail function is called, but it is not imported anywhere in the file

but for example here https://github.com/mobxjs/mobx-state-tree/blob/master/packages/mobx-state-tree/src/core/node/scalar-node.ts

fail function explicitly imported

as i understand it wont work unless fail is in global scope?

coolsoftwaretyler commented 1 year ago

Thanks, @Sinled! That's still quite helpful.

I am going through old issues and triaging, so I appreciate you getting back to me. If you run into it again, a reproduction would be helpful, but it should be easy for us to find this and patch it up with the provided info.

Not sure on the timeline, just getting started with some maintenance here, but I will update this issue as things get prioritized and moved along.

coolsoftwaretyler commented 1 year ago

Hey @nithinssabu - I'm going to assign this to you since you asked to get tasked with some smaller bug fixes. I'd love to see a PR here that adds a test making sure we throw the correct error, and of course fix the missing import.

Let me know if you've got additional questions. Thanks!

coolsoftwaretyler commented 1 year ago

Hmm, for whatever reason, GitHub is not letting me assign this to you. See if you can self-assign, @nithinssabu. Otherwise, this comment should serve as info enough to keep folks from doubling work.

chakrihacker commented 1 year ago

Hey @nithinssabu have you picked this??

coolsoftwaretyler commented 10 months ago

Hey folks, I ended up needing to fix this in order to write some tests of the internals. I'm gonna merge that and close this out. Thanks for reporting it and for your initial interest!