Conversation
| processor(node.storedValue) | ||
| } | ||
|
|
||
| export interface IReflected { |
There was a problem hiding this comment.
maybe IModelReflectionData?
| export function reflect(target: IStateTreeNode): IReflected { | ||
| // check all arguments | ||
| if (process.env.NODE_ENV !== "production") { | ||
| if (!isStateTreeNode(target)) |
There was a problem hiding this comment.
I think we should also check if it is a model type specifically? (not array or map)
There was a problem hiding this comment.
currently it also works with a map - that's also why the subtype
There was a problem hiding this comment.
Never mind, you made that work as well :)
| fail("expected first argument to be a mobx-state-tree node, got " + target + " instead") | ||
| } | ||
| const node: any = getStateTreeNode(target) | ||
| const type = node.type.subType || node.type |
There was a problem hiding this comment.
Remind me: why is the subType needed here :)?
| properties: type.properties, | ||
| actions: Object.keys(node.type.actions || {}), | ||
| volatile: Object.keys(node.type.volatile || {}), | ||
| views: Object.keys(node.type.views || {}) |
There was a problem hiding this comment.
for the views, should we somehow make clear whether a view is a function or a property? Or separately expose as viewProperties and viewFunctions? (implementation wise you could check whether the property descriptor has get )
| if (!isPlainObject(actions)) | ||
| fail(`actions initializer should return a plain object containing actions`) | ||
|
|
||
| if (node.type) addReadOnlyProp(node.type, "actions", actions) |
There was a problem hiding this comment.
2 questions about this approach
- potentially several instances could expose different actions (I mean you could return actions conditionally). However I don't think that matters much, as that would be bad practice anyway
- I think this will break for multiple chained
actionscalls? Or no, probably, it doesn't because each time a new type is produced 🤔 . Let's make sure that is verified by some test.
There was a problem hiding this comment.
let's verify both 👍
There was a problem hiding this comment.
can you make sure that the fields like actions are predeclared on ComplexType? (initialized as utils.EMPTY_ARRAY)
There was a problem hiding this comment.
we might want to change the name to indicate that it is not a reflection of the type a bit more and make it easier to guess what it does e.g. getMembers(node)
might be easier to understand the eventual sugar around it too
e.g.
getMembers(node, !!publicOnly)
getPrivateMembers(node) // if such ever would be introduced
getActions(node)
...
| })) | ||
|
|
||
| test("reflection model", t => { | ||
| const node = Model.create() |
There was a problem hiding this comment.
can you add a test that model({ x: types.string} and then assert reflect(type).properties.x === types.string?
|
Great stuff @robinfehr! I left a bunch of comments & questions, but overal looking great :) |
robinfehr
force-pushed
the
feature/reflection
branch
from
63c8b22 to
087bdd7
Compare
robinfehr
force-pushed
the
feature/reflection
branch
from
72b472e to
400a505
Compare
|
@mweststrate I rewrote that so it's all on the node since with how we currently initialise the members (on the node creation) it's cleaner like that. would be glad if you could review that again.
|
|
Hey @robinfehr, thanks! I like that the reflection is now correct. The risk however is that we make creating object nodes even more expensive (which is already problem). So I played with making the whole reflection api lazy, so that the pain is postponed until somebody actually needs the reflection. The reflection itself is more expensive now (one might want to cache it), but it won't slow down object construction. What do you think? 3184f26 |
|
@mweststrate looking good. cool idea to check for isComputed etc. |
|
Merged! |
2 participants
No description provided.