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.

Sign up for GitHub

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

Jump to bottom

Layer breakers here and there #15293

Closed
jrieken opened this issue Nov 10, 2016 · 18 comments
Closed

Layer breakers here and there #15293

jrieken opened this issue Nov 10, 2016 · 18 comments
Labels
debt Code quality issues
Milestone
November 2016

Comments

@jrieken
Copy link
Member

jrieken commented Nov 10, 2016 •
edited

We have a way of structuring our code in layers and target runtimes. That's why our code is organised into folders like common, browser, node, electron-main, and electron-browser. The idea of target environments is to have easily reusable code. It also means, code from a lower layer isn't allowed to use code from a higher layer.

We don't always stick to that rule and therefore I have added a tslint-rule. It's not yet enabled because there are still too many violations. This issue is a collection of all of them. If you are assigned make sure to move your code into the right folder.

To run the check yourself do this

  • open tslint.json and set layering to true
  • run gulp tslint

After a minute or two, it will produce output like this

src/vs/workbench/workbench.main.ts:15:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:16:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:37:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:43:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:48:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:49:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:58:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:60:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:68:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:69:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:71:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:73:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:75:1:Bad layering. You are not allowed to access 'node' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:77:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:78:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:80:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:84:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:90:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:94:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:96:1:Bad layering. You are not allowed to access 'node' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:98:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:99:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/workbench.main.ts:101:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/api/node/mainThreadTerminalService.ts:8:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, node]
src/vs/workbench/api/node/mainThreadTreeExplorers.ts:10:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common, node]
src/vs/workbench/parts/markers/markers.contribution.ts:6:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/platform/environment/common/environment.ts:6:1:Bad layering. You are not allowed to access 'node' from here, allowed layers are: [common]
src/vs/platform/extensionManagement/common/extensionManagement.ts:13:1:Bad layering. You are not allowed to access 'node' from here, allowed layers are: [common]
src/vs/platform/request/common/request.ts:10:1:Bad layering. You are not allowed to access 'node' from here, allowed layers are: [common]
src/vs/platform/url/common/urlIpc.ts:12:1:Bad layering. You are not allowed to access 'electron-main' from here, allowed layers are: [common]
src/vs/workbench/parts/explorers/common/treeExplorerActions.contribution.ts:11:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common]
src/vs/workbench/parts/explorers/common/treeExplorerActions.ts:10:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common]
src/vs/workbench/parts/git/browser/gitActions.ts:32:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser]
src/vs/workbench/parts/terminal/test/terminalConfigHelper.test.ts:13:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, browser, workbench]
src/vs/workbench/services/viewlet/common/viewletService.ts:11:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common]
src/vs/workbench/test/common/editor/editorStacksModel.test.ts:23:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common]
src/vs/workbench/test/node/quickOpen/quickopen.perf.test.ts:8:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common, node]
src/vs/workbench/test/node/quickOpen/quickopen.perf.test.ts:19:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common, node]
src/vs/workbench/test/node/quickOpen/quickopen.perf.test.ts:29:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common, node]
src/vs/workbench/test/node/quickOpen/quickopen.test.ts:9:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common, node]
src/vs/workbench/test/node/quickOpen/quickopen.test.ts:12:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common, node]
src/vs/platform/configuration/test/common/testConfigurationService.ts:8:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common]
src/vs/platform/telemetry/test/node/telemetryService.test.ts:11:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common, node]
src/vs/workbench/parts/extensions/test/node/extensionsActions.test.ts:13:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common, node]
@jrieken jrieken added the debt Code quality issues label Nov 10, 2016
@jrieken jrieken added this to the November 2016 milestone Nov 10, 2016
jrieken added a commit that referenced this issue Nov 10, 2016
@jrieken
debt - add lint rule to check for layering violations, #15293
d9a83a6
jrieken added a commit that referenced this issue Nov 10, 2016
@jrieken
recompile rule, #15293
44516dd
@jrieken
Copy link
Member Author

jrieken commented Nov 10, 2016 •
edited

Note that the rule does not yet check for importing node-modules outside of node or electron-[main|browser] folders. I'll might implement that as a second rule.

@joaomoreno
Copy link
Member

I love this.

@bpasero
Copy link
Member

bpasero commented Nov 10, 2016

Very cool 👍

bpasero added a commit that referenced this issue Nov 10, 2016
@bpasero
fix some layer breakers for #15293
89cca3a
@jrieken
Copy link
Member Author

jrieken commented Nov 10, 2016 •
edited

This the list of bad node_module imports import-patterns: true

src/vs/editor/common/modes/TMState.ts:9:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/platform/message/common/messageCli.ts:6:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/workbench/parts/explorers/common/treeExplorerViewModel.ts:8:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/workbench/parts/git/browser/gitServices.ts:38:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/platform/extensionManagement/test/common/extensionEnablementService.test.ts:7:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/workbench/parts/search/test/common/searchModel.test.ts:8:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/workbench/parts/search/test/common/searchResult.test.ts:8:1:Imports violates '{**/vs/**,assert}'-restriction.

jrieken added a commit that referenced this issue Nov 10, 2016
@jrieken
add rule for bad node_module imports, #15293
fe21922
jrieken added a commit that referenced this issue Nov 10, 2016
@jrieken
tweak rule and config, #15293
27d8b31
@joaomoreno joaomoreno mentioned this issue Nov 10, 2016
Closed
joaomoreno added a commit that referenced this issue Nov 10, 2016
@joaomoreno
lint: remove semver dependency from gitServices
eee9f26 
#15293
joaomoreno added a commit that referenced this issue Nov 10, 2016
@joaomoreno
lint: fix ParsedArgs layer breakage
5d949f1 
#15293
joaomoreno added a commit that referenced this issue Nov 10, 2016
@joaomoreno
lint: fix extension management layer breakage
d70cfb4 
#15293
joaomoreno added a commit that referenced this issue Nov 10, 2016
@joaomoreno
lint: fix request layer breakage
3e4cce7 
#15293
joaomoreno added a commit that referenced this issue Nov 10, 2016
@joaomoreno
lint: fix urlIpc layer breakage
df7dbe4 
#15293
joaomoreno added a commit that referenced this issue Nov 10, 2016
@joaomoreno
lint: fix gitActions layer breakage
b3b664d 
#15293
@bpasero bpasero removed their assignment Nov 10, 2016
@joaomoreno
Copy link
Member

joaomoreno commented Nov 10, 2016 •
edited

I've fixed the issues from my world. @bpasero appears to have done the same.

Here's the updated gulp tslint log for the next guy:

src/vs/workbench/api/node/mainThreadTerminalService.ts:8:1:Bad layering. You are not allowed to access 'electron-browser' from here, allowed layers are: [common, node]
src/vs/workbench/api/node/mainThreadTreeExplorers.ts:10:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common, node]
src/vs/workbench/parts/explorers/common/treeExplorerActions.contribution.ts:11:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common]
src/vs/workbench/parts/explorers/common/treeExplorerActions.ts:10:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common]
src/vs/workbench/services/viewlet/common/viewletService.ts:11:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common]
src/vs/workbench/parts/extensions/test/node/extensionsActions.test.ts:13:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common, node]

🎆

@joaomoreno joaomoreno removed their assignment Nov 10, 2016
Tyriar added a commit that referenced this issue Nov 10, 2016
@Tyriar
lint: fix mainThreadTerminalService layer breakage
a1eac69 
Part of #15293
@Tyriar Tyriar removed their assignment Nov 10, 2016
@sandy081
Copy link
Member

Fixed

src/vs/workbench/parts/extensions/test/node/extensionsActions.test.ts:13:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common, node]

@Tyriar
Copy link
Member

Tyriar commented Nov 11, 2016

Remaining:

src/vs/workbench/services/viewlet/common/viewletService.ts:11:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common]
src/vs/workbench/parts/extensions/test/browser/extensionsActions.test.ts:14:1:Bad layering. You are not allowed to access 'node' from here, allowed layers are: [common, browser]
src/vs/workbench/parts/extensions/test/browser/extensionsActions.test.ts:19:1:Bad layering. You are not allowed to access 'node' from here, allowed layers are: [common, browser]
src/vs/workbench/parts/extensions/test/browser/extensionsActions.test.ts:21:1:Bad layering. You are not allowed to access 'node' from here, allowed layers are: [common, browser]

@jrieken
Copy link
Member Author

jrieken commented Nov 11, 2016

These are the remaining errors

src/vs/editor/common/modes/TMState.ts:9:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/platform/message/common/messageCli.ts:6:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/platform/extensionManagement/test/common/extensionEnablementService.test.ts:7:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/workbench/services/viewlet/common/viewletService.ts:11:1:Bad layering. You are not allowed to access 'browser' from here, allowed layers are: [common]
src/vs/workbench/parts/explorers/common/treeExplorerViewModel.ts:8:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/workbench/parts/extensions/test/browser/extensionsActions.test.ts:14:1:Bad layering. You are not allowed to access 'node' from here, allowed layers are: [common, browser]
src/vs/workbench/parts/extensions/test/browser/extensionsActions.test.ts:19:1:Bad layering. You are not allowed to access 'node' from here, allowed layers are: [common, browser]
src/vs/workbench/parts/extensions/test/browser/extensionsActions.test.ts:21:1:Bad layering. You are not allowed to access 'node' from here, allowed layers are: [common, browser]
src/vs/workbench/parts/search/test/common/searchModel.test.ts:8:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/workbench/parts/search/test/common/searchResult.test.ts:8:1:Imports violates '{**/vs/**,assert}'-restriction.
[09:41:18] 'tslint' errored after 1.53 min

bpasero added a commit that referenced this issue Nov 11, 2016
@bpasero
one more for #15293
2654eed
@bpasero
Copy link
Member

bpasero commented Nov 11, 2016

I fixed the viewletService one (fyi @octref)

jrieken added a commit that referenced this issue Nov 11, 2016
@jrieken
move TMState into node folder, #15293
24d627d
sandy081 added a commit that referenced this issue Nov 11, 2016
@sandy081
#15293 Layer breakage fixes
bb74b45
sandy081 added a commit that referenced this issue Nov 11, 2016
@sandy081
Layer breakings #15293
17951c3
@sandy081
Copy link
Member

Fixed my parts.. once sinon library is excluded following is left

src/vs/workbench/parts/explorers/common/treeExplorerViewModel.ts:8:1:Imports violates '{**/vs/**,assert}'-restriction.

@sandy081 sandy081 removed their assignment Nov 11, 2016
@joaomoreno joaomoreno removed their assignment Nov 11, 2016
@chrmarti chrmarti removed their assignment Nov 11, 2016
@joaomoreno
Copy link
Member

This is the correct full list:

src/vs/platform/extensionManagement/test/common/extensionEnablementService.test.ts:7:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/workbench/parts/explorers/common/treeExplorerViewModel.ts:8:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/workbench/parts/search/test/common/searchModel.test.ts:8:1:Imports violates '{**/vs/**,assert}'-restriction.
src/vs/workbench/parts/search/test/common/searchResult.test.ts:8:1:Imports violates '{**/vs/**,assert}'-restriction.

@sandy081 @chrmarti Who owns search*.test.ts?

@chrmarti
Copy link
Contributor

@joaomoreno I'll look into these. We got distracted by the partial lists. How does one get the full list?

@chrmarti chrmarti self-assigned this Nov 11, 2016
@joaomoreno
Copy link
Member

Enable this and this by setting them to true and run gulp tslint. 👍

@joaomoreno
Copy link
Member

@sandy081 So what do we do about sinon?

@chrmarti
Copy link
Contributor

The remaining search*.test.ts ones are both sinon. Should this be excluded like assert appears to be as well?

jrieken added a commit that referenced this issue Nov 11, 2016
@jrieken
debt - don't have commonjs-dependencies in common layer, #15293
a39eaec
@jrieken
Copy link
Member Author

jrieken commented Nov 11, 2016

All warnings resolved 👯‍♂️ Thanks everyone.

@jrieken jrieken closed this as completed Nov 11, 2016
@jrieken
Copy link
Member Author

jrieken commented Nov 11, 2016 •
edited

This the configuration of our 3 new rules

        "layering": [
            true,
            {
                "common": [],
                "node": [
                    "common"
                ],
                "browser": [
                    "common"
                ],
                "electron-main": [
                    "common",
                    "node"
                ],
                "electron-browser": [
                    "common",
                    "browser",
                    "node"
                ]
            }
        ],
        "import-patterns": [
            true,
            {
                "target": "**/{node,electron-browser,electron-main,extensions}/**",
                "restrictions": "**/*"
            },
            {
                "target": "{**/**.test.ts,**/test/**}",
                "restrictions": "{**/vs/**,assert,sinon}"
            },
            {
                "target": "**/{common,browser,workbench}/**",
                "restrictions": "**/vs/**"
            }
        ],
        "duplicate-imports": true

@octref octref removed their assignment Nov 11, 2016
@chrmarti chrmarti removed their assignment Dec 7, 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.
Assignees
No one assigned
Labels
debt Code quality issues
Projects
None yet
Milestone
November 2016
Development

No branches or pull requests
9 participants
@joaomoreno @bpasero @jrieken @Tyriar @octref @alexdima @chrmarti @sandy081 @gregvanl