search

mobxjs / mobx-state-tree

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

give complex types debug names #2049

Closed BrianHung closed 11 months ago

BrianHung commented 11 months ago

https://github.com/mobxjs/mobx-state-tree/discussions/2011

This PR gives complex types (array, map, and object) debug names. The human-readable names appear in MobX error messages and in the trace utility. MST already has a method called describe on each complex type that we can reuse for the MobX names.

coolsoftwaretyler commented 11 months ago

Hey @BrianHung - not sure if this PR is ready for review yet or not, and I see you have another open PR as well. Thanks for putting these together!

If you are still working on them, disregard this comment. I just wanted to let you know I saw it and I'm stoked to get 'em merged. I am about to be away from my computer until next week, so I won't have time to look things over. I'll tag @jamonholmgren if he's got time, but I don't know his schedule so I won't speak for him.

Would you mind doing a couple things:

  1. At a minimum, can you write up a description for the PR beyond the link to the discussion? I'd like to make sure we include context in the PR description for other reviewers, and folks reading in the future. It'll also help in case anything from that discussion got changed in your implementation.
  2. If you have a few minutes, I'd also love to see some tests alongside this. If you don't have interest or time to write those tests, you could help out by describing a few different ways that this code could be tested. Maybe a code snippet of intended usage. Adding tests is a great way for new contributors to the repo to get started, but I'd like to be able to provide guidance to them (or to myself, haha).
  3. Of course, I'd be very happy if this included some additional documentation. But again, if you don't have time or interest, no worries on that. Adding docs is a great first contribution we can get volunteers for, much like the tests.

Thanks again! If Jamon doesn't get to this stuff before I'm back, I'll try and take a look next week. Feel free to ping me about it by the end of next week if there's no movement here.

coolsoftwaretyler commented 11 months ago

Thanks for the updated description, @BrianHung! I played around a bit with the code here and it makes sense to me. With CI passing, tests looking good, i'm going to merge it.

I've opened up https://github.com/mobxjs/mobx-state-tree/pull/2049, which I'll work on sourcing someone for in terms of tests and docs. But I think the change is pretty safe as-is, and you'll be able to start using it directly from the next minor release (unsure when that'll actually be).