Skip to content

Change Paths option maxRecursionDepth default value #1029

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Emiyaaaaa
Copy link
Collaborator

For performance improvements in most cases, I think we could reduce the default values of maxRecursionDepth

PS: In TypeScript, also have a similar function to determine whether a type is deeply-nested or not, that value is 5(reduced to 3 after [email protected])
image

@Emiyaaaaa Emiyaaaaa changed the title Change Paths option maxRecursionDepth default value. Change Paths option maxRecursionDepth default value Jan 2, 2025
@@ -53,7 +53,7 @@ export type PathsOptions = {
};

type DefaultPathsOptions = {
maxRecursionDepth: 10;
maxRecursionDepth: 5;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Emiyaaaaa This only affects cases where depth is greater than 5, right?
And this would also be a breaking change for cases where depth is greater than 5, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breaking change

yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only affects cases where depth is greater than 5

yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea, similar to what is mentioned in the description, is that when the depth recursion reaches a certain number (I think it can be 5), it can be assumed that the user input type is an infinite nested type and we can stop the recursion early, the previous 10 is ts maximum limit of recursion for our implementation code

If that's okay, I need to figure out how to explain the a option in the comments.

@sindresorhus
Copy link
Owner

This is a breaking change and should probably be done in v5, right?

@som-sm som-sm mentioned this pull request Jan 9, 2025
@Emiyaaaaa
Copy link
Collaborator Author

This is a breaking change and should probably be done in v5, right?

yes

@LaurynasGr
Copy link
Contributor

LaurynasGr commented Mar 2, 2025

I have a similar idea for this - the main thing I want to acheive is separate the general maximum depth Paths type goes into (right now called maxRecursionDepth) and specifically a circular reference depth.

Once that's done we could keep the general depth larger (e.g. just keep it at 10), but reduce the circular depth to something more aggressive (like 3) for performance. This way we would achieve best of both worlds:

type SimpleDeep = {a: {b: {c: {d: {e: {f: {g: {h: string}}}}}}}}

Paths<SimpleDeep> => 'a' | 'a.b' | 'a.b.c' | .... | 'a.b.c.d.e.f.g.h'

type DeepWithCircular = {
    a: {b: {c: {d: {e: {f: {g: {h: string}}}}}}};
    circular: DeepWithCircular;
}

// Let's say we have 3 as default maxCircularDepth
Paths<DeepWithCircular> =>
    |  'a' | 'a.b' | 'a.b.c' | .... | 'a.b.c.d.e.f.g.h'
    // Note: will not type out 'a' | 'a.b' | 'a.b.c' .... for the ones below but it would have these paths as well
    | 'circular'
    | 'circular.a.b.c.d.e.f.g.h'
    | 'circular.circular'
    | 'circular.circular.a.b.c.d.e.f.g.h'
    | 'circular.circular.circular'
    | 'circular.circular.circular.a.b.c.d.e.f.g' // We hit the default maxRecursionDepth of 10 here
    | 'circular.circular.circular.circular'


// We can set a specific maxCircularDepth
Paths<DeepWithCircular, {maxCircularDepth: 1}> =>
    |  'a' | 'a.b' | 'a.b.c' | .... | 'a.b.c.d.e.f.g.h'
    // Note: will not type out 'a' | 'a.b' | 'a.b.c' .... for the ones below but it would have these paths as well
    | 'circular'
    | 'circular.a.b.c.d.e.f.g.h'
    | 'circular.circular'

I started working on this last week, but jumped on Subrtact & Add negative return values first, as I though I'd need negative values for this one (looks like I will not though).

Thoughts? @som-sm @sindresorhus @Emiyaaaaa

@Emiyaaaaa
Copy link
Collaborator Author

Emiyaaaaa commented Mar 3, 2025

Thoughts? @som-sm @sindresorhus @Emiyaaaaa

Actually, my idea is to limit all default depths, assuming that each attribute has n fields and the allows maximum depth is m. The output would then have n^0 + n^1 + n^2 + ... + n^m paths, if m bigger than 5, it's more likely to cause performance problems, and I think the default of 5 is enough for most scenes.

@som-sm
Copy link
Collaborator

som-sm commented Mar 3, 2025

@Emiyaaaaa The output you're referring to applies to perfect k-ary trees, but IMO, real-world objects are rarely that perfect.

For e.g., the structure below would have 1 + 2 + 4 + 8 + 16 = 31 paths.

                         1                           
                  /             \                    
           2                           3             
        /     \                     /     \          
     4            5             6             7      
    / \          / \           / \           / \     
  8     9     10     11     12     13     14     15  
 / \   / \   /  \   /  \   /  \   /  \   /  \   /  \ 
16 17 18 19 20  21 22  23 24  25 26  27 28  29 30  31

However, real-world objects would often have many nodes missing, something like:

      1
     / \
    2   3
   / \   \
  4   5   6
     / \   \ 
   10   11  13
   /   /  \
 20   22  23

In this case, the total number of paths is only 1 + 2 + 3 + 3 + 3 = 12, much lower than the perfect case.


I think the default of 5 is enough for most scenes

I agree that reducing maxRecursionDepth to 5 can benefit, but since it's a breaking change, it should be introduced in v5.

@sindresorhus sindresorhus mentioned this pull request Mar 3, 2025
14 tasks
@som-sm
Copy link
Collaborator

som-sm commented Mar 3, 2025

Thoughts? @som-sm @sindresorhus @Emiyaaaaa

I agree! In cases involving both circular and non-circular paths, an option like maxCircularDepth can be useful.

While this would still be a breaking change, it would likely go unnoticed in most cases. Because in cases like the following, it'd be rare that people rely on deep chains like parent.parent.parent.parent.pareant.name being present in the result generared by Paths.

type FileSystemNode = {
  name: string;
  type: string;
  parent?: FileSystemNode;
  children?: FileSystemNode[];
};

It’s probably still best to introduce it in v5!

@LaurynasGr
Copy link
Contributor

@som-sm OK, I'll set up the PR to add maxCircularDepth this week.

My plan was to default it to 10 in Paths, but reduce it to 5 in PickDeep with an option to override it from outside:

type PickDeepOptions = Pick<PathsOptions, 'maxCircularDepth'>;
export type PickDeep<T, PathUnion extends Paths<T, Options>, Options extends PickDeepOptions = {maxCircularDepth: 5}> = ....

This would further reduce any possible impact or breaking changes as it would only affect PickDeep and only circular references within it.

Do you think this would still need to wait for v5?

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

Successfully merging this pull request may close these issues.

4 participants