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

Order of building components and isolation - discussion #1663

Closed
qballer opened this issue May 22, 2019 · 13 comments
Closed

Order of building components and isolation - discussion #1663

qballer opened this issue May 22, 2019 · 13 comments

Comments

@qballer
Copy link
Contributor

qballer commented May 22, 2019

I am working on created a descent typescript compiler for the bit. Right now our main issue, that current compiler doesn't emit definition and implementation from the community take a long time to run. In order to resolve that we would like to isolate each component from the workspace, build it in this isolation with type emitting (.d.ts files).

This should be a non issue when working with node modules, but does pose a problem when working with bit components.

Order
We have two components A,B and A --uses-->B, this means that B must be compiled before that in order to have it's type information available when compiling A. In more complex example you reach the conclusion that order of component must be in topological sort.

When we reach the compilation phase of A the code may look something as following:

import {b} from '../B'

I am wondering what is the build order of components inside a bit workspace? I believe that it should be toposorted (perhaps with double pass if we cyclic dependencies)

isolation

When A will be isolated, it doesn't need to have access to the source of B. In the isolation layer it should reach a link file to node_modules (so TS compiler will not try to read B's source code).

import * as ninja from '@bit-[hash]/B'
export default ninja

There in the node_modules there must be a version of B waiting with the type information which was just build.

Would love some input from on what goes on with bit today. and how can we create an API which helps to accomplish that. This presents the case of typescript, but I'm sure these ideas can benefit other sides of the product.

@davidfirst
Copy link
Member

Currently, there is no sort action before building the components. Bit grabs the list of the components from .bitmap and run the build process for each one of them serially.
Check out the function buildAll in src/api/consumer/lib/build.js file if you'd like to change it.

About isolation, there is an option saveDependenciesAsComponents you can pass, which determine whether the dependencies are locally imported or installed as NPM packages.

In general, we do support cyclic dependencies when tagging components.
We had a similar issue where we needed to get the flattened-dependencies (all dependencies, including dependencies of dependencies) of components with cyclic dependencies.
Assume A uses B and B uses A. To resolve A dependencies we had to resolve all B dependencies, but for that, we had to resolve A and so on.
We ended up building a graph. You can see it in src/scope/component-ops/tag-model-component.js file, buildComponentsGraph function.
Hope that helps.

@qballer
Copy link
Contributor Author

qballer commented May 23, 2019

saveDependenciesAsComponents - but I don't want to do that, I want to save components as node modules dependencies, in order to isolate the component.

@GiladShoham
Copy link
Member

@qballer How would you do this?
2 issues:

  1. You have a require statement in the source code to a relative path, if you want to require it from node_modules. you will have to change the source code.
  2. Where in the node_modules will you put the already compiled components? just a random one which deleted at the end of the compilation process? (they might not be exported yet, so we don't know the final node_modules path for them)

@qballer
Copy link
Contributor Author

qballer commented May 23, 2019

  1. You have a require statement in the source code to a relative path, if you want to require it from node_modules. you will have to change the source code.

You will add a link file that will redirect you to the node_modules

  1. Where in the node_modules will you put the already compiled components? just a random one which deleted at the end of the compilation process? (they might not be exported yet, so we don't know the final node_modules path for them)

'@bit-[hash]/B'

@qballer
Copy link
Contributor Author

qballer commented May 28, 2019

It seem that there is a bug. when compiling just aaa component in the uploaded example, bbb doesn't get build and when running a it explodes.

this should be solved by ordering components in topological sort.
bug-cba.zip

@davidfirst @GiladShoham

@GiladShoham
Copy link
Member

@qballer I'm aware of this bug for a long time.
It's indeed a bug, but it's not related to the sort.
We should build dependencies even if we don't sort it first.
But we will fix it as part of the sort PR

@qballer
Copy link
Contributor Author

qballer commented May 28, 2019

It has to be sorted in topological sort to build dependencies correctly.

@davidfirst
Copy link
Member

@qballer , how topological sort will solve cyclic dependencies?

@qballer
Copy link
Contributor Author

qballer commented May 29, 2019

It won't, (though it is a step in the right direction) but it will solve the fact that typescript builds all components and dependencies for each component build. Then with some luck we can reuse component A build in the build of B (which depends on A).

This hinders performance greatly, and doesn't allow TS users to use .d.ts files. I've made some performance improvements that make the community compiler useable but they rely on the main file being the first in the files array. Can this property be supported in bit's current compiler API? (@davidfirst )

Cyclic dependencies can be solved with compiling twice and generating empty module definition for dependencies in the first compile. This requires additional API's for the compiler writer (thats me):

  1. Which dependencies do I build source for ?
  2. Get dependency dist ?

If that dependency has a types file then do nothing, if it doesn't, then generate a generic one in the dist. This should workout performance and cyclic all together.

@GiladShoham GiladShoham added this to the Angular milestone Jun 27, 2019
@davidfirst
Copy link
Member

@qballer , according to your last message, if I understand correctly, there is no need to toposort the components. All you need is the data about the dependencies.

I prepared a small snippet that you can copy/paste to your compiler. It answers your questions # 1 and # 2 above. For this, no changes are needed, you get already all the data inside the componentWithDependencies variable received when creating the capsule.

In addition, I created a new branch feature/export-graph-to-compiler you can use, it's more like a POC branch, just to check the possibility of using the graph in the compiler.
As you'll see in the example below, I added a new property "graph" to the results you get when creating the capsule. This new property exposes the graph (created by graphlib), and also two functions: getEdgesPreorder for pre-order traversal, and getEdgesTopsort for topological sort, which will throw an exception in case of cyclic dependencies.

var compile = function compile(files, distPath, context) {
  return context.isolate('/tmp/my-c').then(({ capsule, componentWithDependencies, graphs }) => {
    componentWithDependencies.dependencies.forEach((dependency) => {
      console.log('--------------------------------------------------')
      console.log('dependency name', dependency.id.toString());
      console.log('dependency rootDir', dependency.writtenPath);
      console.log('dependency files', dependency.files.map(f => f.path).join(', '));
      console.log('dependency has dist? ', !dependency.dists.isEmpty());
      console.log('dependency dist rootDir ', dependency.dists.distsRootDir);
      console.log('dependency dists paths', dependency.dists.get().map(d => d.path).join(', '));
    });
    console.log('**** GRAPH  (you must git checkout to feature/export-graph-to-compiler before using it) ****');
    console.log('get all edges preorder ', graphs.getEdgesPreorder());
    console.log('get all edges toposort ', graphs.getEdgesTopsort());
    ...

An example of the output I got when running it on 3 component (bar/foo requires utils/is-string, which requires utils/is-type.

➜  5uufecdi-local rm -rf /tmp/my-c && bit build --no-cache
⠈⡙ building components--------------------------------------------------
dependency name 117tlnmi-remote/utils/[email protected]
dependency rootDir .dependencies/utils/is-string/117tlnmi-remote/0.0.1
dependency files .dependencies/utils/is-string/117tlnmi-remote/0.0.1/is-string.ts
dependency has dist?  true
dependency dist rootDir  .dependencies/utils/is-string/117tlnmi-remote/0.0.1/dist
dependency dists paths .dependencies/utils/is-string/117tlnmi-remote/0.0.1/is-string.js.map, .dependencies/utils/is-string/117tlnmi-remote/0.0.1/is-string.js
--------------------------------------------------
dependency name 117tlnmi-remote/utils/[email protected]
dependency rootDir .dependencies/utils/is-type/117tlnmi-remote/0.0.1
dependency files .dependencies/utils/is-type/117tlnmi-remote/0.0.1/is-type.ts
dependency has dist?  true
dependency dist rootDir  .dependencies/utils/is-type/117tlnmi-remote/0.0.1/dist
dependency dists paths .dependencies/utils/is-type/117tlnmi-remote/0.0.1/is-type.js.map, .dependencies/utils/is-type/117tlnmi-remote/0.0.1/is-type.js
**** GRAPH ****
get all edges preorder  [ '117tlnmi-remote/bar/[email protected]',
  '117tlnmi-remote/utils/[email protected]',
  '117tlnmi-remote/utils/[email protected]' ]
get all edges toposort  [ '117tlnmi-remote/bar/[email protected]',
  '117tlnmi-remote/utils/[email protected]',
  '117tlnmi-remote/utils/[email protected]' ]
117tlnmi-remote/bar/[email protected]
/private/var/folders/zk/qw7sjckx3gl284cwjmjx57vm0000gn/T/bit/e2e/5uufecdi-local/components/bar/foo/dist/bar/foo.js.map
/private/var/folders/zk/qw7sjckx3gl284cwjmjx57vm0000gn/T/bit/e2e/5uufecdi-local/components/bar/foo/dist/bar/foo.js

@qballer
Copy link
Contributor Author

qballer commented Jul 1, 2019

I was spitballing ideas here but after further considerations this seems to be something bit needs to support - toposorted order of compilations. The way I see it, given a graph of dependencies bit does the traversal on the graph and node processing (a.k.a compilation) is done by the compiler.

@GiladShoham
Copy link
Member

I agree with @qballer . It should be bit's responsibility.
I think we should provide the compiler a way to declare he wants to do toposort when building. most the compilers don't need it, so it's better to give them to decide.
I still think that passing the graph is great since it's useful data that compilers might need.
We should identify case when compiler wants toposort and there are cyclic deps and throw an error in such case (for now)

@davidfirst
Copy link
Member

Implemented on #1781 PR.
It's supported only by the capsule and it throws an exception for cyclic dependencies.
To enable it, pass "shouldBuildDependencies" parameter to the isolate function.
An example: context.isolate({ targetDir: '/tmp/something', shouldBuildDependencies: true });.
Once the capsule is returned to the compiler, all dependencies dists are already written into the capsule.

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

3 participants