Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@ecraig12345
ecraig12345 committed Jan 11, 2022
1 parent 63bd6a1 commit cfa7bb9
Showing 1 changed file with 76 additions and 56 deletions.
132 changes: 76 additions & 56 deletions packages/eslint-plugin/src/rules/ban-imports.js
View file
Expand Up @@ -5,20 +5,22 @@ const createRule = require('../utils/createRule');
// Nasty syntax required for type imports until https://github.com/microsoft/TypeScript/issues/22160 is implemented.
// For some reason just importing TSESTree and accessing properties off that doesn't work.
/**
* @typedef {import("@typescript-eslint/typescript-estree").TSESTree.ImportSpecifier} ImportSpecifier
* @typedef {import("@typescript-eslint/typescript-estree").TSESTree.ImportDeclaration} ImportDeclaration
* @typedef {import("@typescript-eslint/typescript-estree").TSESTree.ExportSpecifier} ExportSpecifier
* @typedef {import("@typescript-eslint/typescript-estree").TSESTree.ExportNamedDeclaration} ExportNamedDeclaration
* @typedef {import("@typescript-eslint/typescript-estree").TSESTree.ExportSpecifier} ExportSpecifier
* @typedef {import("@typescript-eslint/typescript-estree").TSESTree.Identifier} Identifier
* @typedef {import("@typescript-eslint/typescript-estree").TSESTree.ImportDeclaration} ImportDeclaration
* @typedef {import("@typescript-eslint/typescript-estree").TSESTree.ImportSpecifier} ImportSpecifier
*
* @typedef {{
* path?: string;
* pathRegex?: string;
* names?: (string | { regex: string })[];
* message?: string;
* }} OptionsInput
* @typedef {Pick<OptionsInput, 'path' | 'message'> & {
* pathRegex?: RegExp;
* @typedef {{
* path: string | RegExp;
* names?: (string | RegExp)[];
* message?: string;
* }} Options
*/

Expand All @@ -33,8 +35,8 @@ module.exports = createRule({
recommended: false,
},
messages: {
pathNotAllowed: "Importing from '{{path}}' is not allowed{{message}}",
nameNotAllowed: "Importing '{{name}}' from '{{path}}' is not allowed{{message}}",
pathNotAllowed: "{{verb}} from '{{path}}' is not allowed{{message}}",
nameNotAllowed: "{{verb}} '{{name}}' from '{{path}}' is not allowed{{message}}",
},
schema: {
type: 'array',
Expand All @@ -44,11 +46,11 @@ module.exports = createRule({
properties: {
path: {
type: 'string',
description: 'Path or module to ban importing from (non-regex)',
description: 'Path or module to ban importing or re-exporting from (non-regex)',
},
pathRegex: {
type: 'string',
description: 'Regex for path or module to ban importing from',
description: 'Regex for path or module to ban importing or re-exporting from',
},
names: {
type: 'array',
Expand All @@ -64,7 +66,7 @@ module.exports = createRule({
],
},
description:
'If specified, only ban importing these names (if not specified, ban all imports from this path)',
'If specified, only ban importing or re-exporting these names (if not specified, ban all from this path)',
},
message: {
type: 'string',
Expand All @@ -88,61 +90,56 @@ module.exports = createRule({
throw new Error('ban-imports: each objects object must specify only one of `path` or `pathRegex`');
}
return {
path: entry.path,
pathRegex: entry.pathRegex ? new RegExp(entry.pathRegex) : undefined,
path: entry.path || new RegExp(/** @type {string} */ (entry.pathRegex)),
names: entry.names
? entry.names.map(name => (typeof name === 'string' ? name : new RegExp(name.regex)))
: undefined,
};
});

/**
* @param {ImportDeclaration | ExportNamedDeclaration} importOrExport
* @param {string} value
* @param {string | RegExp} stringOrRegex
*/
function checkImportOrExport(importOrExport) {
if (
!importOrExport.source ||
importOrExport.source.type !== AST_NODE_TYPES.Literal ||
typeof importOrExport.source.value !== 'string'
) {
return;
}

const importPath = importOrExport.source.value;
const specifiers =
importOrExport.type === AST_NODE_TYPES.ExportNamedDeclaration
? importOrExport.specifiers
: // Filter out default imports and namespace (star) imports
/** @type {ImportSpecifier[]} */ (importOrExport.specifiers.filter(
spec => spec.type === AST_NODE_TYPES.ImportSpecifier,
));
function isMatch(value, stringOrRegex) {
return typeof stringOrRegex === 'string' ? value === stringOrRegex : stringOrRegex.test(value);
}

/**
* @param {ImportDeclaration | ExportNamedDeclaration} importOrExport the whole import/export node
* @param {string} importPath path importing/exporting from
* @param {Identifier[]} identifiers imported/exported identifiers
*/
function checkImportOrExport(importOrExport, importPath, identifiers) {
for (const rule of options) {
const { path, pathRegex, names, message = '' } = rule;
const pathForErrors = path || /** @type {RegExp} */ (pathRegex).source;
const messageForErrors = message && `: ${message}`;
const { path, names, message } = rule;

if (!isMatch(importPath, path)) {
continue;
}

const errorData = {
path: typeof path === 'string' ? path : path.source,
message: message ? `: ${message}` : '',
verb: importOrExport.type === AST_NODE_TYPES.ImportDeclaration ? 'Importing' : 'Exporting',
};

if ((path && path === importPath) || (pathRegex && pathRegex.test(importPath))) {
if (!names) {
// All imports from this path are banned
context.report({
node: importOrExport,
messageId: 'pathNotAllowed',
data: { path: pathForErrors, message: messageForErrors },
});
} else {
// Only certain imports from this path are banned
for (const spec of specifiers) {
const identifier = spec.type === AST_NODE_TYPES.ExportSpecifier ? spec.local : spec.imported;
if (
names.some(name => (typeof name === 'string' ? name === identifier.name : name.test(identifier.name)))
) {
context.report({
node: identifier,
messageId: 'nameNotAllowed',
data: { path: pathForErrors, name: identifier.name, message: messageForErrors },
});
}
if (!names) {
// All imports from this path are banned
context.report({
node: importOrExport,
messageId: 'pathNotAllowed',
data: errorData,
});
} else {
// Only certain imports from this path are banned
for (const identifier of identifiers) {
if (names.some(name => isMatch(identifier.name, name))) {
context.report({
node: identifier,
messageId: 'nameNotAllowed',
data: { ...errorData, name: identifier.name },
});
}
}
}
Expand All @@ -151,10 +148,33 @@ module.exports = createRule({

return {
ImportDeclaration: imprt => {
checkImportOrExport(imprt);
// imprt.source.value is the import path (should always be a string in practice, but it's
// typed to allow any literal type such as number for some reason)
if (typeof imprt.source.value !== 'string') {
return;
}

const specifiers = /** @type {ImportSpecifier[]} */ (imprt.specifiers.filter(
// Filter out default imports and namespace (star) imports
spec => spec.type === AST_NODE_TYPES.ImportSpecifier,
))
// spec.imported is the original name: `foo` in `import { foo as bar } from 'whatever'`
.map(spec => spec.imported);

checkImportOrExport(imprt, imprt.source.value, specifiers);
},
ExportNamedDeclaration: exprt => {
checkImportOrExport(exprt);
// Verify that the import or export is from a literal path, not an export of a local name
if (exprt.source?.type !== AST_NODE_TYPES.Literal || typeof exprt.source.value !== 'string') {
return;
}

checkImportOrExport(
exprt,
exprt.source.value,
// spec.local is the original name: `foo` in `export { foo as bar } from 'whatever'`
exprt.specifiers.map(spec => spec.local),
);
},
};
},
Expand Down

0 comments on commit cfa7bb9

Please sign in to comment.