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

Recursively import component's dependencies and dependents #1885

Closed
itaymendel opened this issue Jul 31, 2019 · 16 comments
Closed

Recursively import component's dependencies and dependents #1885

itaymendel opened this issue Jul 31, 2019 · 16 comments
Assignees
Milestone

Comments

@itaymendel
Copy link
Contributor

itaymendel commented Jul 31, 2019

Description

When I want to modify a component, I want to have autotag to take care of updating all the dependents of my components. This only happens when the dependent components are sourced to my projects. So I want a way to import all dependents and update them.

Describe the solution you'd like

There are several related workflows:

  1. Ability to view all remote dependents of a component (tree/flatten view).
  2. Ability to view all remote dependencies of a component (tree/flatten view).
  3. Import all component-dependencies of a component to my project, so I can modify any of the dependencies, and when tagging a change autotag will take care of the rest.
  4. Import all component dependents, so a modification I'm doing to a component will quickly propagate to all dependents with a single bit tag command.

Examples:

Syntax
List dependents
bit show bit.utils/array/diff --recursive dependents   # Show remote dependents of a component recursively
bit show bit.utils/array/diff --recursive dependents  --depth 3   # Stop listing dependents when reaching a specific depth in the tree
bit show bit.utils/array/diff  --recursive #   Show remote dependencies of a component recursively (dependencies is the default)
bit show bit.utils/array/diff --recursive --depth 3  # Stop listing dependencies when reaching a specific depth in the tree
Import dependencies as components
bit import bit.utils/array/diff --recursive   # Import immediate remote dependencies of a component (dependencies is the default)
bit import bit.utils/array/diff --recursive dependencies   # explicit ask for dependencies
bit import bit.utils/array/diff --recursive --depth 3   # Stop importing dependencies when reaching a specific depth in the tree, then switch to npm-install)
Import dependents as components
bit import bit.utils/array/diff --recursive dependents  # import remote dependents of a component recursively
bit import bit.utils/array/diff --recursive dependents  --depth 3  # Stop importing dependents when reaching a specific depth in the tree

Additional info

  1. should work with bit import <remote>/* syntax as well.
  2. dependents location should be configurable. Default location is dependents, near the .dependencies folder.
  3. Ensure that imported-dependents will not be dependencies of the project, so when I eject them back after a tag (or even w/o a tag), by default they will not be installed back as packages.

Questions

  1. Should we add a flag to allow Bit to hop between remotes (thus potentially import the entire universe)?
  2. How should we make sure that dependents will not be considered as dependencies for the workspace?
@davidfirst
Copy link
Member

Thanks @itaymendel , that would be a super useful feature!
In general, the approach looks good to me.
My only concern is the .dependents directory in addition to the .dependencies. It's a whole new type of components to manage. Imagine a component that is both a dependent and a dependency of an imported component.

I'm thinking maybe we don't need the dependents at all in the workspace.
The reason we need them is for the auto-tag to increment their dependencies version, so there are no changes to the dependents files only to their objects in the scope.
Therefore, it should be enough to only import the dependents to the scope without writing them into the workspace.

@itaymendel
Copy link
Contributor Author

itaymendel commented Jul 31, 2019

I like the idea. However, we would want to indicate that there are dependents the workspace does not use that are auto-tagged.

Update

  • If we are to import only the objects, will the user be able to checkout a dependent component to their workspace in case of an issue with its build/test?
  • If we import the dependents to another directory, and one of them is also a dependency - it should still be in the dependencies directory. I don't see why this will be a problem.
  • Another question that we should consider is how to handle build/test issues for components that are in the scope but not in the workspace (for example - they are imported to another branch).

@itaymendel itaymendel added this to the 14.3.0 milestone Aug 4, 2019
@Tallyb
Copy link
Contributor

Tallyb commented Aug 6, 2019

How about simplifying the syntax to --dependents (= --recursive dependents) and --dependencies (= --recursive).
Not sure about the display format, but this way you can possibly show both.

@davidfirst
Copy link
Member

@itaymendel , the specific case of the same component as dependency and dependent is not necessarily a problem.
What I'm trying to say is that if we write the components to the workspace, it's going to be a new type of components to deal with.
For example, it will need to be written in the .bitmap file. We currently have author, imported and nested. This will probably be a new one, maybe dependent. Then, we need to go through the logic of nested and see what makes sense for dependent and what not.
Or, what if a component is dependent and then imported directly or the other way around, imported directly and then imported as dependent with another version. Should it be replaced/added. Should we use the same logic as Nested? they're all questions we'll need to deal with.

However, as you mentioned, if we want to build/test, it'll be easier to have them in the workspace. We could use the capsule to just isolate them and run the compiler/tester (and for this, we don't need them in the workspace), but with the current performance, it will be a slow process.

Maybe we should provide two options:

  1. a flag to import all dependents directly, so, they'll be imported type and will be treated as imported. (they'll be written to the workspace like any other imported components).
  2. a flag to import only their objects, so they'll be auto-tag without running their test/build.

About your question regarding the 'checkout', it won't be possible if we only import the objects, because the idea is to not having them in the .bitmap file, so the user will need to import rather than checkout.

@odedre
Copy link
Member

odedre commented Aug 7, 2019

Idea: when recursively importing a component's dependencies, bit should compare with the repo it was exported from to know which dependencies originate from the same project. it can be a nice indication to which components should be auto-imported and which should not.
@GiladShoham

@davidfirst davidfirst self-assigned this Aug 7, 2019
davidfirst added a commit that referenced this issue Aug 10, 2019
#1914)

* introduce new flags --dependents and --dependencies for "bit show" to display them all recursively (implements a part of #1885).

* add e2e-test for the `show` api
@davidfirst
Copy link
Member

The part of bit show is done. Merged into master and added tests.
The flag names are --dependents and --dependencies, according to @Tallyb suggestion and it is possible to use them both at the same command.

I listed them as a table sorted by the depth with the following columns for dependents (dependencies is similar):

Dependent ID - self-explanatory.
Depth - self-explanatory.
Immediate Dependency - since there is no easy way to draw the graph dependencies in the CLI, this column complete the puzzle as to the relation between the nodes.
Dependent type - can be one of the following: prod, dev, compiler and tester.

@itaymendel , I'm unsure about the --depth flag. I didn't implement it and am not convinced it's needed. It doesn't help in terms of performance because the entire graph needs to be built regardless of the depth. It can only help for the display to not show too many dependencies/dependents. I checked a few components from bit.envs, which is a quite big and connected graph and didn't get too much information.

@Tallyb
Copy link
Contributor

Tallyb commented Aug 10, 2019

That looks great @davidfirst
Regarding depth, and in general - is it going to show the first time the dependency's depth, or are we going to have all instances? (i.e. a -> b, b -> c , a-> c - how is C going to be displayed? What will be shown as an immediate dependency?

I would suggest in the dependencies and dependents tables to remove every time the component itself is shown with "(self)" or something similar, instead of repeating it. It makes a quick scan of the lists much easier.

@davidfirst
Copy link
Member

@Tallyb , it shows the dependencies only once with no duplications. The depth is the shortest path from the component to the dependency in the graph. Under the hood, it uses Dijkstra's algorithm to find it.
To confirm the case you mentioned above, I created a workspace with those components and as expected, it shows C only once and with a depth of 1. See screenshot.

Screen Shot 2019-08-12 at 2 55 51 PM

About changing the immediate dependency of the component itself to something else, I'm open to it. @itaymendel , what do you think?

@Tallyb
Copy link
Contributor

Tallyb commented Aug 13, 2019

@davidfirst @itaymendel - what do you think about showing all instances, and not just the first one to give more detailed info?

@davidfirst
Copy link
Member

About the flags for bit import, in the PR above I have implemented them both --dependents and --dependencies to work the same.
Meaning, the components are imported directly (IMPORTED in .bitmap file) and written into the workspace.
For dependencies, it works as described above.
For dependents, we didn't come to a final conclusion yet as to what would be the best experience. However, it was the easiest to implement and it's better than what we have now. (currently, users import the dependents manually). Also, that's the only way to get the test/build functionality with no extra work.
@itaymendel , please let me know if you have objections.

@Tallyb , by 'instances' you mean 'depth'? if so, we do show all depths already.

@Tallyb
Copy link
Contributor

Tallyb commented Aug 14, 2019

@davidfirst - by instances, I mean that in the above example comp-c will be shown in 2 separate lines, once for a, and once for b.

davidfirst added a commit that referenced this issue Aug 15, 2019
…rt" (#1927)

* implements #1885, introduce new flags "--dependents" and "--dependencies" for "bit import" to import them all directly

* fix a case when importing a component with --dependent flag and the component has the same dependent with different versions to import only the highest version

* fix the graph to include all versions and not only the latest
@davidfirst
Copy link
Member

@Tallyb , we're thinking to add the functionality of generating an image with the entire graph. It'll be easy to see all this data in the graph.
For the table of bit show, I believe the data we display now is sufficient.

@davidfirst
Copy link
Member

The flags --dependents and --dependencies forbit import are implemented as discussed above and merged into master.

@GiladShoham
Copy link
Member

I'm fine with @Tallyb suggestion about "self" it will be easier. we might want to somehow differentiate it from regular names. (color / bold / italic / capital) for making it easier to scan.
Also what if I have a component called self?

@Tallyb
Copy link
Contributor

Tallyb commented Sep 3, 2019

then <self>. or better <this> (we are javascript after all...|).
You will not have a component with <>.
Italic is cool.

@davidfirst
Copy link
Member

Done. I added it as <self>.
I believe that all features of this issue are implemented. Closing.

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

No branches or pull requests

5 participants