Skip to content

Commit

Permalink
feat(schematics): show warnings about importing lazy loadable librari…
Browse files Browse the repository at this point in the history
…es without forcing users configure them in tslint.json
  • Loading branch information
vsavkin committed Mar 13, 2018
1 parent e87faf5 commit 56788ba
Show file tree
Hide file tree
Showing 6 changed files with 249 additions and 159 deletions.
7 changes: 4 additions & 3 deletions e2e/schematics/command-line.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ describe('Command line', () => {
() => {
newProject();
newApp('myapp');
newApp('myapp2');
newLib('mylib');
newLib('lazylib');

const tslint = JSON.parse(readFile('tslint.json'));
tslint.rules['nx-enforce-module-boundaries'][1].lazyLoad.push('lazylib');
updateFile('tslint.json', JSON.stringify(tslint, null, 2));

updateFile(
Expand All @@ -19,8 +19,9 @@ describe('Command line', () => {
import '../../../libs/mylib';
import '@proj/lazylib';
import '@proj/mylib/deep';
import '@proj/myapp';
import '@proj/myapp/main';
import '@proj/myapp2';
const s = {loadChildren: '@proj/lazylib'};
`
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@
"nx-enforce-module-boundaries": [
true,
{
"lazyLoad": [],
"allow": []
}
]
Expand Down
2 changes: 1 addition & 1 deletion packages/schematics/src/collection/workspace/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ function updateTsLintJson(options: Schema) {
json[key] = undefined;
});
json.rulesDirectory.push('node_modules/@nrwl/schematics/src/tslint');
json['nx-enforce-module-boundaries'] = [true, { lazyLoad: [], allow: [] }];
json['nx-enforce-module-boundaries'] = [true, { allow: [] }];
});
return host;
};
Expand Down
2 changes: 1 addition & 1 deletion packages/schematics/src/command-line/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ function getFilesFromShash(sha1: string, sha2: string): string[] {
.filter(a => a.length > 0);
}

function getProjectNodes(config) {
export function getProjectNodes(config) {
return (config.apps ? config.apps : []).filter(p => p.name !== '$workspaceRoot').map(p => {
return {
name: p.name,
Expand Down
266 changes: 167 additions & 99 deletions packages/schematics/src/tslint/nxEnforceModuleBoundariesRule.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,122 +2,217 @@ import { RuleFailure } from 'tslint';
import * as ts from 'typescript';

import { Rule } from './nxEnforceModuleBoundariesRule';
import {Dependency, DependencyType, ProjectNode, ProjectType} from '../command-line/affected-apps';

describe('Enforce Module Boundaries', () => {
it('should not error when everything is in order', () => {
const failures = runRule(
{ allow: ['@mycompany/mylib/deep'] },
`${process.cwd()}/proj/apps/myapp/src/main.ts`,
`
import '@mycompany/mylib';
import '@mycompany/mylib/deep';
import '../blah';
`
import '@mycompany/mylib';
import '@mycompany/mylib/deep';
import '../blah';
`,
[
{
name: 'myapp',
root: 'libs/myapp/src',
type: ProjectType.app,
files: [
`apps/myapp/src/main.ts`,
`apps/myapp/blah.ts`
]
},
{
name: 'mylib',
root: 'libs/mylib/src',
type: ProjectType.lib,
files: [
`libs/mylib/index.ts`,
`libs/mylib/deep.ts`
]
}
]
);

expect(failures.length).toEqual(0);
});

it('should not error when lib name prefix collides with name of lazy loaded lib', () => {
const failures = runRule({ lazyLoad: ['mylib'] }, `import '@mycompany/mylib-not-lazy';`);
expect(failures.length).toEqual(0);
});

describe('relative imports', () => {
it('should not error when relatively importing the same library', () => {
const failures = runRuleToCheckForRelativeImport('import "../mylib2"');
const failures = runRule(
{},
`${process.cwd()}/proj/libs/mylib/src/main.ts`,
'import "../other"',
[
{
name: 'mylib',
root: 'libs/mylib/src',
type: ProjectType.lib,
files: [
`libs/mylib/src/main.ts`,
`libs/mylib/other.ts`
]
}
]
);
expect(failures.length).toEqual(0);
});

it('should not error when relatively importing the same library (index file)', () => {
const failures = runRuleToCheckForRelativeImport('import "../../mylib"');
expect(failures.length).toEqual(0);
});

it('should not error when relatively importing the same library (lib name prefix collision)', () => {
const failures = runRuleToCheckForRelativeImport(
'import "../some-comp"',
'/proj/libs/dir/mylib2/src/module.t'
const failures = runRule(
{},
`${process.cwd()}/proj/libs/mylib/src/main.ts`,
'import "../other"',
[
{
name: 'mylib',
root: 'libs/mylib/src',
type: ProjectType.lib,
files: [
`libs/mylib/src/main.ts`,
`libs/mylib/other/index.ts`
]
}
]
);
expect(failures.length).toEqual(0);
});

it('should error when relatively importing another library', () => {
const failures = runRuleToCheckForRelativeImport('import "../../../libs/mylib2"');
expect(failures.length).toEqual(1);
expect(failures[0].getFailure()).toEqual('library imports must start with @mycompany/');
});

it('should error on a relatively importing another library (in the same dir)', () => {
const failures = runRuleToCheckForRelativeImport('import "../../mylib2"');
expect(failures.length).toEqual(1);
expect(failures[0].getFailure()).toEqual('library imports must start with @mycompany/');
const failures = runRule(
{},
`${process.cwd()}/proj/libs/mylib/src/main.ts`,
'import "../other"',
[
{
name: 'mylib',
root: 'libs/mylib/src',
type: ProjectType.lib,
files: [`libs/mylib/src/main.ts`]
},
{
name: 'other',
root: 'libs/other/src',
type: ProjectType.lib,
files: []
}
]
);
expect(failures[0].getFailure()).toEqual(
'library imports must start with @mycompany/'
);
});
});

it('should error on absolute imports into libraries without using the npm scope', () => {
const failures = runRule({}, `import 'libs/mylib';`);
const failures = runRule(
{},
`${process.cwd()}/proj/libs/mylib/src/main.ts`,
'import "libs/src/other.ts"',
[
{
name: 'mylib',
root: 'libs/mylib/src',
type: ProjectType.lib,
files: [
`libs/mylib/src/main.ts`,
`libs/mylib/src/other.ts`
]
}
]
);

expect(failures.length).toEqual(1);
expect(failures[0].getFailure()).toEqual('library imports must start with @mycompany/');
expect(failures[0].getFailure()).toEqual(
'library imports must start with @mycompany/'
);
});

it('should error about deep imports into libraries', () => {
const failures = runRule({}, `import '@mycompany/mylib/blah';`);

expect(failures.length).toEqual(1);
expect(failures[0].getFailure()).toEqual('deep imports into libraries are forbidden');
});

it('should not error about deep imports when libs contain the same prefix', () => {
let failures = runRule(
const failures = runRule(
{},
`import '@mycompany/reporting-dashboard-ui';
import '@mycompany/reporting-other';
import '@mycompany/reporting';
`,
['reporting', 'reporting-dashboard-ui']
`${process.cwd()}/proj/libs/mylib/src/main.ts`,
'import "@mycompany/other/blah"',
[
{
name: 'mylib',
root: 'libs/mylib/src',
type: ProjectType.lib,
files: [`libs/mylib/src/main.ts`]
},
{
name: 'other',
root: 'libs/other/src',
type: ProjectType.lib,
files: [`libs/other/blah.ts`]
}
]
);

expect(failures.length).toEqual(0);

// Make sure it works regardless of order of app names list
failures = runRule(
{},
`import '@mycompany/reporting-dashboard-ui';
import '@mycompany/reporting-other';
import '@mycompany/reporting';`,
['reporting-dashboard-ui', 'reporting']
expect(failures[0].getFailure()).toEqual(
'deep imports into libraries are forbidden'
);

expect(failures.length).toEqual(0);
});

it('should error on importing a lazy-loaded library', () => {
const failures = runRule({ lazyLoad: ['mylib'] }, `import '@mycompany/mylib';`);

expect(failures.length).toEqual(1);
const failures = runRule(
{},
`${process.cwd()}/proj/libs/mylib/src/main.ts`,
'import "@mycompany/other";',
[
{
name: 'mylib',
root: 'libs/mylib/src',
type: ProjectType.lib,
files: [`libs/mylib/src/main.ts`]
},
{
name: 'other',
root: 'libs/other/src',
type: ProjectType.lib,
files: [`libs/other/index.ts`]
}
],
{
mylib: [{projectName: 'other', type: DependencyType.loadChildren}]
}
);
expect(failures[0].getFailure()).toEqual('imports of lazy-loaded libraries are forbidden');
});

it('should error on importing an app', () => {
const failures = runRule({ lazyLoad: ['mylib'] }, `import '@mycompany/myapp';`, [], ['myapp']);

expect(failures.length).toEqual(1);
expect(failures[0].getFailure()).toEqual('imports of apps are forbidden');
});

it('should error on importing a lib that has the same prefix as an app', () => {
const noFailures = runRule({ lazyLoad: [] }, `import '@mycompany/myapp/mylib';`, ['myapp/mylib'], ['myapp']);

expect(noFailures.length).toEqual(0);
const failures = runRule(
{},
`${process.cwd()}/proj/libs/mylib/src/main.ts`,
'import "@mycompany/myapp"',
[
{
name: 'mylib',
root: 'libs/mylib/src',
type: ProjectType.lib,
files: [`libs/mylib/src/main.ts`]
},
{
name: 'myapp',
root: 'apps/myapp/src',
type: ProjectType.app,
files: [`apps/myapp/index.ts`]
}
]
);
expect(failures[0].getFailure()).toEqual(
'imports of apps are forbidden'
);
});
});

function runRule(
ruleArguments: any,
contentPath: string,
content: string,
libNames: string[] = ['mylib'],
appNames: string[] = []
projectNodes: ProjectNode[],
deps: { [projectName: string]: Dependency[] } = {}
): RuleFailure[] {
const options: any = {
ruleArguments: [ruleArguments],
Expand All @@ -126,38 +221,11 @@ function runRule(
};

const sourceFile = ts.createSourceFile(
'proj/apps/myapp/src/main.ts',
contentPath,
content,
ts.ScriptTarget.Latest,
true
);
const rule = new Rule(options, 'proj', 'mycompany', libNames, appNames, []);
return rule.apply(sourceFile);
}

function runRuleToCheckForRelativeImport(
content: string,
sourceFilePath = '/proj/libs/dir/mylib/src/module.t'
): RuleFailure[] {
const options: any = {
ruleArguments: [{}],
ruleSeverity: 'error',
ruleName: 'enforceModuleBoundaries'
};

const sourceFile = ts.createSourceFile(
sourceFilePath,
content,
ts.ScriptTarget.Latest,
true
);
const rule = new Rule(
options,
'/proj',
'mycompany',
['dir/mylib', 'dir/mylib2'],
[],
['libs/dir/mylib', 'libs/dir/mylib2']
);
const rule = new Rule(options, `${process.cwd()}/proj`, 'mycompany', projectNodes, deps);
return rule.apply(sourceFile);
}
}
Loading

0 comments on commit 56788ba

Please sign in to comment.