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

Minor type adjustments for all() and children #86

Open
daKmoR opened this issue Sep 14, 2021 · 1 comment
Open

Minor type adjustments for all() and children #86

daKmoR opened this issue Sep 14, 2021 · 1 comment

Comments

@daKmoR
Copy link

daKmoR commented Sep 14, 2021

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch tree-model@1.0.7 for the project I'm working on.

  1. Allow usage of node.all() which currently throws a typing error as it always expects at least 1 option
  2. Type node.children so when iterating you do not get a typing error

Here is the diff that solved my problem:

diff --git a/node_modules/tree-model/types/index.d.ts b/node_modules/tree-model/types/index.d.ts
index 6f102c9..6ef264f 100644
--- a/node_modules/tree-model/types/index.d.ts
+++ b/node_modules/tree-model/types/index.d.ts
@@ -27,7 +27,7 @@ declare namespace TreeModel {
         walk(options: Options, fn: NodeVisitorFunction<T>, ctx?: object): void;
         walk(fn: NodeVisitorFunction<T>, ctx?: object): void;
 
-        all(options: Options, fn: NodeVisitorFunction<T>, ctx?: object): Array<Node<T>>;
+        all(options?: Options, fn?: NodeVisitorFunction<T>, ctx?: object): Array<Node<T>>;
         all(fn: NodeVisitorFunction<T>, ctx?: object): Array<Node<T>>;
 
         first(options: Options, fn: NodeVisitorFunction<T>, ctx?: object): Node<T> | undefined;
@@ -36,6 +36,7 @@ declare namespace TreeModel {
         drop(): Node<T>;
 
         [propName: string]: any;
+        children?: Array<Node<T>>
     }
 
     interface Config {

This issue body was partially generated by patch-package.

Would you like to have a PR for that?

@joaonuno
Copy link
Owner

Hi!

I'm wondering if removing the mandatory predicate function from all() might cause issues if we then call it with just the strategy: all({strategy: "post"}). Can you confirm this?

If this is non-breaking, please open the PR for the master branch and then I'll fwd port it to next.

Also pls add a test for all without args and update the README to mention that the predicate in all is optional.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants