getFirstAncestor with a lambda

[lazarljubenovic]

Is your feature request related to a problem? Please describe.

I need to get the enclosing method in a class -- but this could be a constructor as well. Looks like we're missing a way to provide a custom lambda for getFirstAncestor, as there is only getFirstAncestorByKind and getFirstAncestorByKindOrThrow.

Describe the solution you'd like

const method = methodBody.getFirstAncestor(ancestor => TypeGuards.isMethodDeclaration(ancestor) || TypeGuards.isConstructorDeclaration(ancestor))

Describe alternatives you've considered

The "workaround" is currently this:

  const ancestors = methodBody.getAncestors()
  const method = ancestors.find(ancestor => TypeGuards.isMethodDeclaration(ancestor) || TypeGuards.isConstructorDeclaration(ancestor))

It's nothing drastic, but this first fetches all ancestors which could be computationally expensive.

[cancerberoSgx]

+1 and also for getDescendants (if it's not there yet). Ideally for all getters that might need refined filtering for example anInterface.getMethods() (signatures overrides have same name), getParameters, etc. @dsherret Can I take this ? Also do you think the name should be getFirstAncestor or findAncestor ? What about filtering ( I don't think is important though since writing getDescendants().filter() is kinda intuitive will end up with the same performance?

[dsherret]

@cancerberoSgx I'm going to take a look. I need to re-evaluate the getParentWhile method. There's already a .getFirstDescendant method.

I definitely don't want to add it on everything because then the library becomes too bloated. This should be a case-by-case addition. For example, .getParameters() uses the node.parameters array already and there are lots of existing methods on an array that people can use to find things. Additionally, there's already a .getParameter(...) and .getParameterOrThrow(...) method. There's also .getMethod(...) method.

I think doing a .filter() on .getDescendants() would be about the same performance. For the best performance, people should use .forEachDescendant((node, traversal) => {}); using the traversal object (see https://dsherret.github.io/ts-simple-ast/navigation/).

[lazarljubenovic]

Indeed, I think that it's more straight-forward for people to do .getParameters().find() than to know about getParametersBy().

The places where these help are most useful are when there are potentially a lot of nodes and when a regular way to do it would be via a while (current != null) { current = current.whatever } type of loop. It really saves typing to have a short-circuited .getAllAndFind(lambda) helper on those (since .getAll.fillter() could be too expensive).

And with TypeGuards maybe we can deprecate getByKind and just use getBy, since it's pretty much the same:

getByKind(SyntaxKind.Something)
getBy(TypeGuard.isSomething)

[cancerberoSgx]

And with TypeGuards maybe we can deprecate getByKind and just use getBy, since it's pretty much the same...

I don't think I agree with that because I could be filtering using other information besides kind, for example, nodes with a certain Identifier text (name) - no matter its kind - or am I missing something ?

[lazarljubenovic]

Exactly. That's why I meant to remove "getters" which get only by syntax kind -- instead, always use a lambda -- and that lambda can be a type guard.

---

In the concrete example, my idea is to remove this type of call completely.

getFirstAncestorByKind(SyntaxKind.ClassDeclaration)

Instead, only provide a function which accepts a lambda and let me pass in any kind of predicate.

getFirstAncestorBy(ancestor => ancestor.getKind() == SyntaxKind.ClassDeclaration)

Or shorter:

getFirstAncestorBy(ancestor => TypeGuards.isClassDeclaration(ancestor))

Or shorter:

getFirstAncestorBy(TypeGuads.isClassDeclaration)

---

The point is, if you look at current thing and the last snippet above, the last one is more flexibile and shorter for the same use case:

getFirstAncestorByKind(SyntaxKind.ClassDeclaration)
getFirstAncestorBy(TypeGuads.isClassDeclaration)

So my suggestion is to remove getFirstAncestorByKind completely as it only bloats the library with a use-case which can already be implemented in just as straight-forward manner and with even less typing.

[cancerberoSgx]

@lazarljubenovic agreed, sorry I misunderstood your previous comment. IMO just the name "findAncestor" would be better than getFirstAncestorBy since Array.prototype.find returns the first item found and is the behavior people would expect for something named "find". Thanks!

[lazarljubenovic]

Indeed, find sounds too. Didn't give the name much thought 😄 And no need to apologize; glad we came to a good solution 👍

[dsherret]

Will be in the next release. I don't want to use find because it wouldn't be consistent with the rest of the library.