search

pkgjs / support

Package Support Format
MIT License
24 stars 7 forks source link

Improve tree printing - full flat tree #16

Closed mhdawson closed 4 years ago

mhdawson commented 4 years ago

See https://github.com/pkgjs/support/pull/12 for more details.

We still don't print out the right output for the structure in: https://github.com/rodion-arr/support/tree/flat_node_modules/test/cli/show-local-tree-flat/node_modules

mhdawson commented 4 years ago

@rodion-arr you had already started to investigate, should I assign to you?

rodion-arr commented 4 years ago

Yes, I'll try to dive a bit deeper to understand what's going on inside buildIdealTree()

rodion-arr commented 4 years ago

Looks like depth property which we rely on represents physical folder structure, not supplied by dependencies tree based on package.json. Here parent for dep3 from specified example is root node_modules, so it is getting depth=1, according to its physical location.

We can do a workaround and try to rebuild depth properties based on edgesOut Map which contains dependencies of current node. Not sure that this solution will be stable.

@mhdawson what do you think, is it worth trying?

ljharb commented 4 years ago

npm itself is basically removing depth where possible; really everyone either wants "1" or "infinity". I wouldn't worry about anything in between those two.

mhdawson commented 4 years ago

@rodion-arr if we followed edgesout instead of children would that get us what we want (even if there is some additional duplication) ?

rodion-arr commented 4 years ago

I mean for this case we will need to build tree by ourselves according to data in edgesOut

mhdawson commented 4 years ago

@rodion-arr, what I was wondering if in this code:

  for (const child of module.children.values()) {
    await printModule(child, depth + 1, argv);
  }

If it might be as simple as iterating over the edgesout instead of children? Have not had time to try that out yet and thought you might know based on your look at arborist.

rodion-arr commented 4 years ago

@mhdawson, this may work! The only problem I see here is that edgesOut is Map of Edges not Nodes, so they have different set of fields. But it looks like we can use edge.to property with reference to relevant Node instance.

I'll try this idea and update

mhdawson commented 4 years ago

@rodion-arr sounds good. Thanks for trying it out !

We may get packages being duplicated under several deps but I think that's ok as from my perspective that would provide an easier to consume picture of the deps.