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

Layer breakers in our code #15558

Closed
jrieken opened this issue Nov 16, 2016 · 9 comments
Closed

Layer breakers in our code #15558

jrieken opened this issue Nov 16, 2016 · 9 comments
Labels
important Issue identified as high-priority

Comments

@jrieken
Copy link
Member

jrieken commented Nov 16, 2016

There is https://github.com/Microsoft/vscode/blob/master/src/vs/platform/textmodelResolver/common/resolver.ts with a dependency onto import { IModel } from 'vs/editor/common/editorCommon'; which isn't legal. It should live in editor/common/services

@jrieken jrieken added the important Issue identified as high-priority label Nov 16, 2016
@bpasero
Copy link
Member

bpasero commented Nov 16, 2016

@jrieken good catch. I found more though:

  • vs/platform/actions/common/resourceContextKey => vs/editor/common/services/modeService
  • vs/platform/extensionManagement/test/common/extensionEnablementService.test => vs/workbench/services/storage/common/storageService
  • vs/editor/contrib/indentation/common/indentation.ts => vs/workbench/services/quickopen/common/quickOpenService
  • vs/editor/contrib/suggest/electron-browser/snippetCompletion.ts => vs/workbench/services/quickopen/common/quickOpenService

Maybe we could enrich the layeringRule to more rules:

  • base: []
  • platform: [base]
  • editor: [platform, base]
  • workbench: [platform, base, editor]

@bpasero bpasero changed the title Layer breaker with textmodelResolver Layer breakers in our code Nov 16, 2016
@bpasero bpasero removed their assignment Nov 16, 2016
jrieken added a commit that referenced this issue Nov 18, 2016
@jrieken
Copy link
Member Author

jrieken commented Nov 18, 2016

  • vs/editor/contrib/suggest/electron-browser/snippetCompletion.ts => vs/workbench/services/quickopen/common/quickOpenService

I think that shows a nice problem with our layering. We somehow don't only separate between target runtimes (JS, browser, electron-main,browser) but also between platform, editor, workbench, base. This is like a 2-dimensional layering and I wonder if that makes things artificially complicated.

In the sample above I wanna contain all suggest-related features in one folder. That makes it easy to find/navigate/understand. Also I have structured it along the runtimes (common/browser/electron) but it conflicts with the goal of also structuring along workbench-editor et al. Somehow conflicting goals and abstraction for little gain. I wonder if we can give/reduce the 2 layer in some way

@bpasero
Copy link
Member

bpasero commented Nov 18, 2016

I think what we used to do in that case is to put the editor contribution into the vs/workbench/parts folder because it can only work if the workbench is there. But yeah, that moves it away from snippets in general.

@jrieken
Copy link
Member Author

jrieken commented Nov 18, 2016

To express the same intent I have put it into the electron-browser folder. The workbench is the only thing that runs from there.

@sandy081
Copy link
Member

I am not sure what is the good solution for tests here. For eg extensionEnablementService has a dependency on storageService where both interfaces are in platform. But implementation of storageService lies in workbench.

Either,

  • Exclude this for tests because they need to instantiate the implementations themselves.
  • Create a mock of such services in tests. This would be tedious. Tests will be more reliable if using real implementations
  • Make sure definitions and implementations remain in the same layer. I think StorageService implementation can be moved as all its dependencies are in platform.

In this case, I would go for last option. But in general I would allow tests to instantiate the implementations.

@alexdima alexdima assigned isidorn and unassigned alexdima Dec 2, 2016
@alexdima
Copy link
Member

alexdima commented Dec 2, 2016

src/vs/editor/contrib/indentation/common/indentation.ts layer break introduced by @isidorn in f01ee98

isidorn added a commit that referenced this issue Dec 5, 2016
@isidorn isidorn removed their assignment Dec 5, 2016
jrieken added a commit that referenced this issue Dec 12, 2016
jrieken added a commit that referenced this issue Dec 12, 2016
@jrieken
Copy link
Member Author

jrieken commented Dec 12, 2016

@joaomoreno and @bpasero: product.ts and package.ts layering issues. Requires nodejs and respective json files.

@jrieken jrieken assigned joaomoreno and bpasero and unassigned jrieken Dec 12, 2016
@bpasero bpasero removed their assignment Dec 12, 2016
joaomoreno added a commit that referenced this issue Dec 12, 2016
@joaomoreno joaomoreno removed their assignment Dec 12, 2016
@joaomoreno
Copy link
Member

@jrieken Fixed those.

@sandy081
Copy link
Member

Moved storage service to package layer.

@jrieken All issues reported here are fixed. You can close this.

@sandy081 sandy081 removed their assignment Dec 12, 2016
@jrieken jrieken closed this as completed Dec 12, 2016
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
important Issue identified as high-priority
Projects
None yet
Development

No branches or pull requests

6 participants