diff --git a/change/@fluentui-eslint-plugin-e581f6ba-1682-40b1-9526-48c87c70a3f0.json b/change/@fluentui-eslint-plugin-e581f6ba-1682-40b1-9526-48c87c70a3f0.json new file mode 100644 index 0000000000000..2fbb9402858dc --- /dev/null +++ b/change/@fluentui-eslint-plugin-e581f6ba-1682-40b1-9526-48c87c70a3f0.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Expand ban-imports rule to also support exports, and support regular expressions for names", + "packageName": "@fluentui/eslint-plugin", + "email": "elcraig@microsoft.com", + "dependentChangeType": "none" +} diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index fde1468be0e04..4feecd7fe5264 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -18,13 +18,13 @@ Helpers for customizing configuration are exported under a `configHelpers` objec ### `ban-imports` -Ban importing from certain paths or modules. You can either ban the entire path, or only certain names. (Inspired by TSLint's [`import-blacklist`](https://palantir.github.io/tslint/rules/import-blacklist/).) +Ban importing or re-exporting from certain paths or modules. You can either ban the entire path, or only certain names. (Inspired by TSLint's [`import-blacklist`](https://palantir.github.io/tslint/rules/import-blacklist/).) Requires one or more options objects. Either `path` or `pathRegex` is required. - `path` (`string`): Path or module to ban importing from (non-regex) - `pathRegex` (`string`): Regex for path or module to ban importing from -- `names` (`string[]`, optional): If provided, only ban imports of these names. Otherwise, ban all imports from the path. +- `names` (`(string | { regex: string })[]`, optional): If provided, only ban imports of these names and/or regular expressions. Otherwise, ban all imports from the path. - `message` (`string[]`, optional): Custom message to show with errors Example: @@ -33,7 +33,7 @@ Example: "@fluentui/ban-imports": [ "error", { "path": "lodash" }, - { "path": "foo", "names": ["bar", "baz"] }, + { "path": "foo", "names": ["bar", { "regex": "^baz" }] }, { "pathRegex": "^\.", message: "no relative imports" }, { "pathRegex": "^\.\./(foo|bar)$", "names": ["baz"] } ] diff --git a/packages/eslint-plugin/src/rules/ban-imports.js b/packages/eslint-plugin/src/rules/ban-imports.js index 3724c50f56cf8..3c2b152fce9d7 100644 --- a/packages/eslint-plugin/src/rules/ban-imports.js +++ b/packages/eslint-plugin/src/rules/ban-imports.js @@ -5,10 +5,23 @@ 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.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[]; message?: string; }} OptionsInput - * @typedef {{ path?: string; pathRegex?: RegExp; names?: string[]; message?: string; }} Options + * @typedef {{ + * path?: string; + * pathRegex?: string; + * names?: (string | { regex: string })[]; + * message?: string; + * }} OptionsInput + * @typedef {{ + * path: string | RegExp; + * names?: (string | RegExp)[]; + * message?: string; + * }} Options */ /** */ @@ -17,13 +30,13 @@ module.exports = createRule({ meta: { type: 'problem', docs: { - description: 'Ban importing certain identifiers from certain paths or modules.', + description: 'Ban importing (or re-exporting) certain identifiers from certain paths or modules.', category: 'Best Practices', 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', @@ -33,17 +46,27 @@ 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', - items: { type: 'string' }, + items: { + oneOf: [ + { type: 'string' }, + { + type: 'object', + properties: { + regex: { type: 'string', description: 'Regex for names to ban' }, + }, + }, + ], + }, 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', @@ -67,51 +90,91 @@ 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, - names: entry.names, + 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 {string} value + * @param {string | RegExp} stringOrRegex + */ + 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, 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 (names) { + // 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 }, + }); + } + } + } else { + // All imports from this path are banned + context.report({ + node: importOrExport, + messageId: 'pathNotAllowed', + data: errorData, + }); + } + } + } + return { ImportDeclaration: imprt => { - if (imprt.source.type !== AST_NODE_TYPES.Literal || typeof imprt.source.value !== 'string') { + // 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 importPath = imprt.source.value; - // Filter out default imports and namespace (star) imports const specifiers = /** @type {ImportSpecifier[]} */ (imprt.specifiers.filter( + // Filter out default imports and namespace (star) imports spec => spec.type === AST_NODE_TYPES.ImportSpecifier, - )); - - for (const rule of options) { - const { path, pathRegex, names, message = '' } = rule; - const pathForErrors = path || /** @type {RegExp} */ (pathRegex).source; - const messageForErrors = message && `: ${message}`; + )) + // spec.imported is the original name: `foo` in `import { foo as bar } from 'whatever'` + .map(spec => spec.imported); - if ((path && path === importPath) || (pathRegex && pathRegex.test(importPath))) { - if (!names) { - // All imports from this path are banned - context.report({ - node: imprt, - messageId: 'pathNotAllowed', - data: { path: pathForErrors, message: messageForErrors }, - }); - } else { - // Only certain imports from this path are banned - for (const spec of specifiers) { - if (names.includes(spec.imported.name)) { - context.report({ - node: spec.imported, - messageId: 'nameNotAllowed', - data: { path: pathForErrors, name: spec.imported.name, message: messageForErrors }, - }); - } - } - } - } + checkImportOrExport(imprt, imprt.source.value, specifiers); + }, + ExportNamedDeclaration: 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), + ); }, }; }, diff --git a/packages/react-components/.eslintrc.json b/packages/react-components/.eslintrc.json index ceea884c70dcc..4b12a5aeb5fb8 100644 --- a/packages/react-components/.eslintrc.json +++ b/packages/react-components/.eslintrc.json @@ -1,4 +1,19 @@ { "extends": ["plugin:@fluentui/eslint-plugin/react"], - "root": true + "root": true, + "overrides": [ + { + "files": "**/index.ts", + "rules": { + "@fluentui/ban-imports": [ + "error", + { + "pathRegex": ".*", + "names": [{ "regex": "Commons$" }], + "message": "Commons types should not be exported from @fluentui/react-components" + } + ] + } + } + ] } diff --git a/scripts/beachball/index.ts b/scripts/beachball/index.ts index 007ce1ed70775..eb855e1bcef90 100644 --- a/scripts/beachball/index.ts +++ b/scripts/beachball/index.ts @@ -11,6 +11,7 @@ export const config: BeachballConfig = { '**/*.{shot,snap}', '**/*.{test,spec}.{ts,tsx}', '**/*.stories.tsx', + '**/.eslintrc.*', '**/__fixtures__/**', '**/__mocks__/**', '**/common/isConformant.ts',