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

feat(linter): add allowedExternalImports option to boundaries rule #13891

Merged
merged 5 commits into from
Dec 21, 2022
Merged

feat(linter): add allowedExternalImports option to boundaries rule #13891

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1f557fd
chore(linter): add empty tests
yjaaidi Dec 17, 2022
fbf238b
chore(linter): add wip tests for allowedExternalImports
yjaaidi Dec 17, 2022
d50512d
feat(linter): add allowedExternalImports option
yjaaidi Dec 17, 2022
fcfbb70
docs(linter): add allowedExternalImports option to the ban external i…
yjaaidi Dec 17, 2022
9569a87
chore(linter): tidy up
yjaaidi Dec 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions docs/shared/recipes/ban-external-imports.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,43 @@ Another common example is ensuring that util libraries stay framework-free by ba
// ... more ESLint config here
}
```

## Whitelisting external imports with `allowedExternalImports`

If you need a more restrictive approach, you can use the `allowedExternalImports` option to ensure that a project only imports from a specific set of packages.
This is useful if you want to enforce separation of concerns _(e.g. keeping your domain logic clean from infrastructure concerns, or ui libraries clean from data access concerns)_ or keep some parts of your codebase framework-free or library-free.

```jsonc {% fileName=".eslintrc.json" %}
{
// ... more ESLint config here

// nx-enforce-module-boundaries should already exist at the top-level of your config
"nx-enforce-module-boundaries": [
"error",
{
"allow": [],
// update depConstraints based on your tags
"depConstraints": [
// limiting the dependencies of util libraries to the bare minimum
// projects tagged with "type:util" can only import from "date-fns"
{
"sourceTag": "type:util",
"allowedExternalImports": ["date-fns"]
},
// ui libraries clean from data access concerns
// projects tagged with "type:ui" can only import pacages matching "@angular/*" except "@angular/common/http"
{
"sourceTag": "type:ui",
"allowedExternalImports": ["@angular/*"],
"bannedExternalImports": ["@angular/common/http"]
},
// keeping the domain logic clean from infrastructure concerns
// projects tagged with "type:core" can't import any external packages.
{
"sourceTag": "type:core",
"allowedExternalImports": []
}
]
}
]
```
100 changes: 100 additions & 0 deletions packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,106 @@ describe('Enforce Module Boundaries (eslint)', () => {
expect(failures[1].message).toEqual(message);
});

it('should not error when importing npm packages matching allowed external imports', () => {
const failures = runRule(
{
depConstraints: [
{ sourceTag: 'api', allowedExternalImports: ['npm-package'] },
],
},
`${process.cwd()}/proj/libs/api/src/index.ts`,
`
import 'npm-package';
import('npm-package');
`,
graph
);

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

it('should error when importing npm packages not matching allowed external imports', () => {
const failures = runRule(
{
depConstraints: [
{ sourceTag: 'api', allowedExternalImports: ['npm-package'] },
],
},
`${process.cwd()}/proj/libs/api/src/index.ts`,
`
import 'npm-awesome-package';
import('npm-awesome-package');
`,
graph
);

const message =
'A project tagged with "api" is not allowed to import the "npm-awesome-package" package';
expect(failures.length).toEqual(2);
expect(failures[0].message).toEqual(message);
expect(failures[1].message).toEqual(message);
});

it('should not error when importing npm packages matching allowed glob pattern', () => {
const failures = runRule(
{
depConstraints: [
{ sourceTag: 'api', allowedExternalImports: ['npm-awesome-*'] },
],
},
`${process.cwd()}/proj/libs/api/src/index.ts`,
`
import 'npm-awesome-package';
import('npm-awesome-package');
`,
graph
);

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

it('should error when importing npm packages not matching allowed glob pattern', () => {
const failures = runRule(
{
depConstraints: [
{ sourceTag: 'api', allowedExternalImports: ['npm-awesome-*'] },
],
},
`${process.cwd()}/proj/libs/api/src/index.ts`,
`
import 'npm-package';
import('npm-package');
`,
graph
);

const message =
'A project tagged with "api" is not allowed to import the "npm-package" package';
expect(failures.length).toEqual(2);
expect(failures[0].message).toEqual(message);
expect(failures[1].message).toEqual(message);
});

it('should error when importing any npm package if none is allowed', () => {
const failures = runRule(
{
depConstraints: [{ sourceTag: 'api', allowedExternalImports: [] }],
},
`${process.cwd()}/proj/libs/api/src/index.ts`,
`
import 'npm-package';
import('npm-package');
`,
graph
);

const message =
'A project tagged with "api" is not allowed to import the "npm-package" package';
expect(failures.length).toEqual(2);
expect(failures[0].message).toEqual(message);
expect(failures[1].message).toEqual(message);
});

it('should error when importing transitive npm packages', () => {
const failures = runRule(
{
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ export default createESLintRule<Options, MessageIds>({
},
],
onlyDependOnLibsWithTags: [{ type: 'string' }],
allowedExternalImports: [{ type: 'string' }],
bannedExternalImports: [{ type: 'string' }],
notDependOnLibsWithTags: [{ type: 'string' }],
},
Expand Down
28 changes: 20 additions & 8 deletions packages/eslint-plugin-nx/src/utils/runtime-lint-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@ type SingleSourceTagConstraint = {
sourceTag: string;
onlyDependOnLibsWithTags?: string[];
notDependOnLibsWithTags?: string[];
allowedExternalImports?: string[];
bannedExternalImports?: string[];
};
type ComboSourceTagConstraint = {
allSourceTags: string[];
onlyDependOnLibsWithTags?: string[];
notDependOnLibsWithTags?: string[];
allowedExternalImports?: string[];
bannedExternalImports?: string[];
};
export type DepConstraint =
Expand Down Expand Up @@ -209,9 +211,23 @@ function isConstraintBanningProject(
externalProject: ProjectGraphExternalNode,
constraint: DepConstraint
): boolean {
return constraint.bannedExternalImports.some((importDefinition) =>
parseImportWildcards(importDefinition).test(
externalProject.data.packageName
const { allowedExternalImports, bannedExternalImports } = constraint;
const { packageName } = externalProject.data;

/* Check if import is banned... */
if (
bannedExternalImports?.some((importDefinition) =>
parseImportWildcards(importDefinition).test(packageName)
)
) {
return true;
}

/* ... then check if there is a whitelist and if there is a match in the whitelist. */
return (
allowedExternalImports != null &&
Copy link
Contributor

@meeroslav meeroslav Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comparison by value got me confused for a bit. Can we replace it with just allowedExternalImports && ...?

Perhaps it would be cleaner to use elvis + every to be in sync with bannedExeternalImports:

return allowedExternalImports?.every(importDefinition =>
  !parseImportWildcards(importDefinition).test(packageName)
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comparison by value got me confused for a bit. Can we replace it with just allowedExternalImports && ...?

I just like to avoid the boolean coercion and make it explicit that I am not mistakenly (thinking that [] == false just like '' == false) trying to check that the array is not empty.
That said, I'm totally alright with the coercing instead and going with allowedExternalImports && ....

Now, concerning optional chaining + every, the problem is that it doesn't make it very explicit that we are expecting allowedExternalImports = [] to return true... which actually happens because [].every(() => true) returns true.
That said, the current implementation !allowedExternalImports.some(...) doesn't make it more expclit 😅.

Anyway, there is a test that makes it clear we are expecting allowedExternalImports = [] to make any external package import fail so I am ok with any implementation.
We can switch to allowedExternalImports?.every(...) (which ends up being my favorite).

!allowedExternalImports.some((importDefinition) =>
parseImportWildcards(importDefinition).test(packageName)
)
);
}
Expand All @@ -230,11 +246,7 @@ export function hasBannedImport(
tags = [c.sourceTag];
}

return (
c.bannedExternalImports &&
c.bannedExternalImports.length &&
tags.every((t) => (source.data.tags || []).includes(t))
);
return tags.every((t) => (source.data.tags || []).includes(t));
meeroslav marked this conversation as resolved.
Show resolved Hide resolved
});
return depConstraints.find((constraint) =>
isConstraintBanningProject(target, constraint)
Expand Down