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

getParentOfType(node, parentType) #477

Closed
xaviergonz opened this issue Oct 19, 2017 · 12 comments
Closed

getParentOfType(node, parentType) #477

xaviergonz opened this issue Oct 19, 2017 · 12 comments
Labels
enhancement Possible enhancement help/PR welcome Help/Pull request from contributors to fix the issue is welcome

Comments

@xaviergonz
Copy link
Contributor

xaviergonz commented Oct 19, 2017

Will try to get the first parent found of a given node that matches a given type, or undefined if none found.

Basically I'm using getParent with different depths to achieve the same effect at the moment, but I'm starting to loose track :) (plus that implementation would be more resiliant to model changes)

@mweststrate
Copy link
Member

mweststrate commented Oct 21, 2017 via email

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Oct 21, 2017

Right now I'm using the following code:

export function getParentOfType<S, M>(target: IStateTreeNode, type: IType<S, M>): M | undefined {
  let currentNode = target;
  while (hasParent(currentNode)) {
    currentNode = getParent(currentNode);
    if (getType(currentNode) === type) {
      return currentNode as M;
    }
  }
  return undefined;
}

But I have a few doubts. What does getParent do? the docs say:

  • Returns the immediate parent of this object, or null.

but then the actual method does:
return fail(Failed to find the parent of ${getStateTreeNode(target)} at depth ${depth})

so, should getParent be changed so it returns null? should its docs be changed?

should the new method return null if not found? undefined? throw?
if throw, should there be a "hasParentOfType" equivalent?

@the-noob
Copy link
Contributor

the-noob commented Nov 23, 2017

Just hit this issue with something like

const Model = types.model('Model', {
  name: types.string,
})
  .views((self) => ({
    get builder() {
       // return getParent(self); - this returns the `models` Map
       return getParent(getParent(self)); // - returns the Builder model
    }
  ));

const Builder = types.model('Builder', {
  models: types.map(Model)
});

Indeed the Map is the first parent but not exactly what I've expected.

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Nov 26, 2017

The code above works. I'm just waiting for @mweststrate or @mattiamanzati to get me a hint on the desired semantics to adapt it and make a pull request

@mweststrate
Copy link
Member

@xaviergonz sorry for late response! PR is welcome, I would go for the two function api version (has and get), so that the get variant can be strict-null typechecked which is quite convienient :)

@xaviergonz
Copy link
Contributor Author

so then get method will throw if there's no such parent right?

@mweststrate
Copy link
Member

mweststrate commented Jan 15, 2018 via email

@optimatex
Copy link

@mweststrate but this approach leads to harder tests implementations since in my tests files I always have to pass the top store of the entire tree, else it will break test run. I followed the bookshop implementation:

.views(self => ({
    get appStore() {
      return getParent(self);
    },
  }))

is there any workaround to prevent throwing error?

@mweststrate
Copy link
Member

mweststrate commented Mar 10, 2018 via email

@optimatex
Copy link

oh, I didn't see this method. Thanks and sorry for this

@robinfehr robinfehr mentioned this issue Mar 21, 2018
@robinfehr robinfehr changed the title Suggestion: getParentOfType(node, parentType) getParentOfType(node, parentType) Mar 21, 2018
@robinfehr robinfehr reopened this Mar 21, 2018
@robinfehr
Copy link
Contributor

on the todo list now.

@robinfehr robinfehr added help/PR welcome Help/Pull request from contributors to fix the issue is welcome enhancement Possible enhancement labels Mar 21, 2018
@mweststrate
Copy link
Member

PR has been merged to master now, so will be released soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Possible enhancement help/PR welcome Help/Pull request from contributors to fix the issue is welcome
Projects
None yet
Development

No branches or pull requests

5 participants