feat: add dynamic object type from luo-occ

coolsoftwaretyler commented 4 months ago

What does this PR do and why?

Supercedes https://github.com/mobxjs/mobx-state-tree/pull/1859 because of pretty rough merge conflicts.

Resolves https://github.com/mobxjs/mobx-state-tree/issues/415 by providing a dynamic object type.

Steps to validate locally

We still need to write tests to exercise the new type, along with documentation for it.

coolsoftwaretyler commented 4 months ago

Hey folks, I'm sorry to say but I don't think this PR is ready to merge. If someone disagrees, I'd love to learn more about what I've missed here. I know at one point we marked this original PR as good to go, but I have found a variety of issues with it this morning.

As it stands, the custom type this creates looks a lot more like our src/types/complex-types/model.ts, rather than a JavaScript object or even MobX dynamic objects.

From the examples provided in the original PR, the main functionality it has is accessing properties directly, rather than with get, like you might in an MSTMap: src/types/complex-types/map.ts.

But we can already do that with the model type. The model type also has better TypeScript support and a variety of helpers (it is, in many ways, the bread and butter of our runtime type system).

In order to move forward with this proposal, I think we need to articulate the value of the dynamic object type, and implement some APIs specific to Objects that our model types do not have.

Object APIs

In __tests__/core/object-type.test.ts, I categorized the JS Object API into three buckets:

APIs we would definitely need to make this type valuable
APIs we might consider implementing for completeness sake or utility
APIs that are clearly meant for JavaScript objects, and we really shouldn't (and probably can't?) implement ourselves.

Additional implementation details

We may want to do more than just the JS object methods, but I figure that's a good enough place to start.

Missing documentation

Before we can merge this, I think we also need to take time to write why it's useful and explain different use cases between model types, map types, and this proposed object type. This step may actually need to come before we do any of the other work.

Missing Type Inference

The original PR was written a while back, so I don't think it had the tools for TS inference we have in so many of our other types. In my opinion, this is a strong requirement. You can reproduce these issues in the test file at __tests__/core/object-type.test.ts. I did not include them since they're sort of weird to include as half-written code.

Compare to t.map:

/**
 * TS tells us:
 * (property) map: <IAnyType>(subtype: IAnyType) => IMapType<IAnyType>
 */
const myMap = t.map()

/**
 * No type hints on this
 * (property) object: any
 */
const myObject = t.object()

/**
 * TS tells us:
 * const myMap: IMapType<ISimpleType<string>>
 */
const myMap = t.map(t.string)

/**
 * We get:
 * const myObject: any
 */
const myObject = t.object(t.string)

/**
 * If you type this in a TS-enhanced editor, you will see:
 * - create
 * - describe
 * - flags
 * - getSnapshot
 * - getSubTypes
 * - hooks
 * - identifierAttribute?
 * - instantiate
 * - is
 * - isAssignableFrom
 * - isType
 * - name
 * - reconcile
 * - validate
 */
myMap. // Auto-complete here

/**
 * This does not happen with myObject
 */
myObject. // No auto-complete suggestions

Next steps

I'm going to leave this as a draft for now so folks can consider it if they're interested in working on it. I'd be happy to consider alternative implementation plans

constgen commented 4 months ago

Hi. Thanks for your work.

Is it a complete list of all points that need to be done? What is the strategy to try to work on it? Should we make another PR to this base branch?

coolsoftwaretyler commented 4 months ago

Hey @constgen - I think this is a pretty comprehensive set of requirements, although I personally don't have a need for this API, so I'd defer to people who are looking for it, such as yourself.

I've written three sets of tests, most of which currently fail. I would say a good approach here would be:

Make the must have tests pass
Ensure type inference works
Evaluate the items listed as "to consider" implementing and pass those tests.
Write documentation (edit: this actually may need to be done first, again, I don't have a strong need for this feature, and I'd love to understand the use cases better so I can provide better guidance)

If you're interested in helping but still not sure exactly how to start, let me know specific questions you have and I'll do my best to answer.

mobxjs / mobx-state-tree