Skip to content
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

Merged
merged 4 commits into from
Mar 8, 2018
Merged

Conversation

TimHollies
Copy link

Exposes 'getIdentifier' function to resolve #674.

/**
 * Returns the identifier of the target node. If a property is supplied returns the identifier
 * of the property even if it is a reference to a missing object.
 *
 * @export
 * @param {IStateTreeNode} target
 * @param {string} property
 * @returns {(string | null)}
 */
export function getIdentifier<T extends IStateTreeNode>(target: T, property?: keyof T): string | null;

Tim Hollies added 3 commits February 27, 2018 15:28
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
Copy link
Contributor

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?

Copy link
Author

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
Copy link
Contributor

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 :)

Copy link
Author

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.

@robinfehr
Copy link
Contributor

robinfehr commented Feb 28, 2018

quick review @TimHollies
cc @mweststrate

  • the property param that is passed to getChildNode(property) seems to be a workaround around getStateTreeNode(node) that fails on resolving the reference.
  • i think the current implementation would only work for identifieRreferences, can that be verified?
  • i'm not so sure if getting identifiers of non resolvable nodes is a good thing in general.
    since to support customRefs and custom types we'd would probably have to use getSnapshot.

for the use case:
if i got that right - you have e.g. cats stored and you want to lazily load the owner on create(), but not using a custom ref. since they wouldn't eagerly load the owner on creation.
would what i've commented on #641 help?

@mweststrate
Copy link
Member

Thanks for the PR @TimHollies! I think @robinfehr's comments were spot on, so thanks for improving :). Will merge and release!

Thanks :)

@mweststrate mweststrate merged commit 9a858cd into mobxjs:master Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants