-
Notifications
You must be signed in to change notification settings - Fork 639
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
feat: add dynamic object type from luo-occ #2157
base: master
Are you sure you want to change the base?
Conversation
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 From the examples provided in the original PR, the main functionality it has is accessing properties directly, rather than with 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 APIsIn
Additional implementation detailsWe may want to do more than just the JS object methods, but I figure that's a good enough place to start. Missing documentationBefore 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 InferenceThe 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 Compare to /**
* 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 stepsI'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 |
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? |
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:
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. |
What does this PR do and why?
Supercedes #1859 because of pretty rough merge conflicts.
Resolves #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.