[plug-in] fix circular dependency warnings in plugin system

[evidolob]

What it does

Fixes circular dependency warnings which appears during webpack build for files in @theia/plugin-ext extension. Also it removes reexport from node folder in common/index.ts file.

How to test

Just build and make sure that build output doesn't contain:

@theia/example-browser:     WARNING in Circular dependency detected:
@theia/example-browser:     ../../packages/plugin-ext/lib/common/index.js -> ../../packages/plugin-ext/lib/plugin/plugin-context.js -> ../../packages/plugin-ext/lib/plugin/quick-open.js -> ../../packages/plugin-ext/lib/common/index.js
@theia/example-browser:
@theia/example-browser:     WARNING in Circular dependency detected:
@theia/example-browser:     ../../packages/plugin-ext/lib/plugin/plugin-context.js -> ../../packages/plugin-ext/lib/plugin/quick-open.js -> ../../packages/plugin-ext/lib/common/index.js -> ../../packages/plugin-ext/lib/plugin/plugin-context.js
@theia/example-browser:
@theia/example-browser:     WARNING in Circular dependency detected:
@theia/example-browser:     ../../packages/plugin-ext/lib/plugin/quick-open.js -> ../../packages/plugin-ext/lib/common/index.js -> ../../packages/plugin-ext/lib/plugin/plugin-context.js -> ../../packages/plugin-ext/lib/plugin/quick-open.js
@theia/example-browser:
@theia/example-browser:     WARNING in Circular dependency detected:
@theia/example-browser:     ../../packages/plugin-ext/lib/plugin/tree/tree-views.js -> ../../packages/plugin-ext/lib/common/index.js -> ../../packages/plugin-ext/lib/plugin/plugin-context.js -> ../../packages/plugin-ext/lib/plugin/tree/tree-views.js

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[kittaakos]

I can confirm, the mentioned cyclic dependency warnings are gone, but I can still see warnings from the plug-ins. Is it known?

@theia/example-electron: WARNING in Circular dependency detected:
1376@theia/example-electron: ../../packages/plugin-ext/lib/hosted/browser/hosted-plugin.js -> ../../packages/plugin-ext/lib/main/browser/main-context.js -> ../../packages/plugin-ext/lib/main/browser/debug/debug-main.js -> ../../packages/plugin-ext/lib/hosted/browser/hosted-plugin.js
1377@theia/example-electron: WARNING in Circular dependency detected:
1378@theia/example-electron: ../../packages/plugin-ext/lib/main/browser/debug/debug-main.js -> ../../packages/plugin-ext/lib/hosted/browser/hosted-plugin.js -> ../../packages/plugin-ext/lib/main/browser/main-context.js -> ../../packages/plugin-ext/lib/main/browser/debug/debug-main.js
1379@theia/example-electron: WARNING in Circular dependency detected:
1380@theia/example-electron: ../../packages/plugin-ext/lib/main/browser/main-context.js -> ../../packages/plugin-ext/lib/main/browser/debug/debug-main.js -> ../../packages/plugin-ext/lib/hosted/browser/hosted-plugin.js -> ../../packages/plugin-ext/lib/main/browser/main-context.js

See the logs on Travis.

[evidolob]

@kittaakos No, maybe I miss that, thx, will take a look

[vince-fugnitto]

@evidolob I assume it fixes the following issue? #5388

[evidolob]

@vince-fugnitto Yes, that one.

[vinokurig]

Works for me

[evidolob]

@kittaakos Now all cyclic dependency warnings in plugin-ext should gone, can you check?

[vinokurig]

works good for me

[azatsarynnyy]

checked and I see no circular dependency in plugin-ext

[akosyakov]

Is there a way to avoid boilerplate interfaces and symbols or it is somehow required to break the cycle?

btw i like MainPluginService, it would be good to rename HostedPluginSupport to it and change a file name as well.

[evidolob]

For me no, at least I cannot think up a better solution.

For name, yes I plan to rename everything in plugin-ext with this plan #3871 (comment)

[akosyakov]

I'm actually not sure what is so bad in cyclic dependencies between classes which should be used together. Is there a conceptual reasoning behind breaking such cycle?

Maybe we just should get rid of cycle dependency check? And have better check where we say such module cannot use such module because of such conceptual reasons, like we do for project organization. Current check to me is an artificial constraint.

Otherwise we introduce indirections with interfaces and symbols not clear for that cause? If there is not a real cause let's rather discuss getting rid of a check on next dev meeting?

[evidolob]

I'm actually not sure what is so bad in cyclic dependencies between classes which cannot be used together. Is there a conceptual reasoning behind breaking such cycle?

No, just desire to remove cyclic dependencies warnings.

Maybe we just should get rid of cycle dependency check? And have better check where we say >such module cannot use such module because such conceptual reasons, like we do for project >organization.

Otherwise we introduce indirections with interfaces and symbols not clear for that cause? If there >is not a real cause let's rather discuss getting rid of a check on next dev meeting?

+1 to discuss, we have a lot of cyclic dependencies warnings in core extension either

In general I'm fine with first commit in this PR, but as for second, yes it just refactoring to resolve cyclic dependencies, and introducing new interface is not very good. So I can revert last commit.
And we can discuss what we will do with all cyclic dependencies in project.

[akosyakov]

@evidolob added to next dev meeting agenda

[akosyakov]

@evidolob first commit is good, it removes a dependency from common to node modules, we want it 👍

[evidolob]

OK, I'm going to delete second commit from this PR

[akosyakov]

i think it is good, index file of common module does not pull nodejs modules anymore