-
Notifications
You must be signed in to change notification settings - Fork 642
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
Added 'getIdentifier' function #678
Conversation
The version of yarn provided by default on TravisCI now supports workspaces - there is no need to load an earlier version. Installing the other yarn version was also breaking the CI builds as it installed to a directory TravisCI did not expect.
*/ | ||
export function getIdentifier<T extends IStateTreeNode>( | ||
target: T, | ||
property?: keyof T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not so sure the property param is needed - omitting it could clean up the code and would be less error prone since string literals can't be checked on e.g. renaming a property.
was there a specific use case you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property param is for the use case in my original issue #641.
The idea is that you can access the identifier that is being searched for by a reference, regardless of whether that identifier exists in the tree or not.
I want to be able to do this so I can lazily load references. This can be done using Custom References, but loading them on first access is a bit unpredictable, which I don't like. I know which references are required for each view, so I'd rather load them all at the start in one place. Currently you have to snapshot each object in order to find out the identifiers that need loading - which is not the intended usage of the snapshot feature.
I've added some tests in packages/mobx-state-tree/test/node.ts
which cover these cases.
@@ -1,20 +1,9 @@ | |||
language: node_js | |||
sudo: required | |||
before_install: # if "install" is overridden |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there already is a separate PR addressing this problem :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I thought there might be - I just wanted to be able to check if it would pass the CI tests.
quick review @TimHollies
for the use case: |
Thanks for the PR @TimHollies! I think @robinfehr's comments were spot on, so thanks for improving :). Will merge and release! Thanks :) |
Exposes 'getIdentifier' function to resolve #674.