search

mobxjs / mobx-state-tree

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

Fix/get members 2173 #2174

Closed evinosheaforward closed 2 months ago

evinosheaforward commented 2 months ago

What does this PR do and why?

Fix reported issue with actions being reported as views in getMembers: https://github.com/mobxjs/mobx-state-tree/issues/2173

issue was bad if/else if causing actions to be returned as views.

The issues

Steps to validate locally

For brute force exploring the differences, I took the existing v5.1.0 reflection tests and ran them vs both 5.1.0 and master and compared the views, properties, volatiles, and actions. This did yield a result of extra views on master (extra view for each action), so I started a git bisect looking for the commit where that changed and found 62e7e8bafe8bf07f4694e429f734bb12323fd8ef as the "bad commit".

Then I updated the tests to fail in this PR. Finally, the fix commit passes the new reflection tests

coolsoftwaretyler commented 2 months ago

Thanks @evinosheaforward! I will take a look today or early in the week. I expect we'll merge soon and ship a patch to v5, and include in v6.

evinosheaforward commented 2 months ago

Thanks @evinosheaforward! I will take a look today or early in the week. I expect we'll merge soon and ship a patch to v5, and include in v6.

Cool - sounds great!

I'll note that there may be another bug reported in #2173 this PR fixes actions -> views but not views -> volatiles ~I'll look into the latter and you can lmk if you want to ship the fixes separately or together~

edit: made a comment on the issue pointing out that the behavior for the views -> volatiles in the reported issue seems correct. Hopefully you can double check me on that.