diff --git a/change/@fluentui-eslint-plugin-bae0af9c-456e-420c-9560-08c80175e2de.json b/change/@fluentui-eslint-plugin-bae0af9c-456e-420c-9560-08c80175e2de.json new file mode 100644 index 00000000000000..bfbe80251cd648 --- /dev/null +++ b/change/@fluentui-eslint-plugin-bae0af9c-456e-420c-9560-08c80175e2de.json @@ -0,0 +1,7 @@ +{ + "type": "none", + "comment": "chore: new eslint rule: `ban-context-export`", + "packageName": "@fluentui/eslint-plugin", + "email": "lingfangao@hotmail.com", + "dependentChangeType": "none" +} diff --git a/change/@fluentui-react-context-selector-e5b506ef-63ca-4690-8e76-eac9690a95f6.json b/change/@fluentui-react-context-selector-e5b506ef-63ca-4690-8e76-eac9690a95f6.json new file mode 100644 index 00000000000000..5c4df3e235214c --- /dev/null +++ b/change/@fluentui-react-context-selector-e5b506ef-63ca-4690-8e76-eac9690a95f6.json @@ -0,0 +1,7 @@ +{ + "type": "none", + "packageName": "@fluentui/react-context-selector", + "comment": "Add eslint ignore for context export", + "email": "lingfangao@hotmail.com", + "dependentChangeType": "none" +} diff --git a/change/@fluentui-react-utilities-5fa1f06a-3abd-457a-b2b4-0dd403f861fd.json b/change/@fluentui-react-utilities-5fa1f06a-3abd-457a-b2b4-0dd403f861fd.json new file mode 100644 index 00000000000000..c8e4b327fff8d5 --- /dev/null +++ b/change/@fluentui-react-utilities-5fa1f06a-3abd-457a-b2b4-0dd403f861fd.json @@ -0,0 +1,7 @@ +{ + "type": "none", + "comment": "Add eslint-ignore", + "packageName": "@fluentui/react-utilities", + "email": "lingfangao@hotmail.com", + "dependentChangeType": "none" +} diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 71aac12a6f9d5e..10231018fcf4d1 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -17,6 +17,36 @@ Helpers for customizing configuration are exported under a `configHelpers` objec ## Rules +### `ban-context-export` + +Exporting context objects as a part of the public API can lead to unexpected usages of context by customers and might +impede future refactoring. To allow customers use context while encapsulating our internals correctly, the developer +should export a provider and hook. + +**❌ Don't** + +```ts +// src/context.ts +import * as React from 'react'; +export const MyContext = React.createContext(); + +// src/index.ts +export { MyContext } from './context'; +``` + +**✅ Do** + +```ts +// src/context.ts +import * as React from 'react'; +const MyContext = React.createContext(); +export const MyContextProvider = MyContext.Provider; +export const useMyContext = () => React.useContext(MyContext); + +// src/index.ts +export { MyContextProvider, useMyContext } from './context'; +``` + ### `ban-imports` 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/).) diff --git a/packages/eslint-plugin/package.json b/packages/eslint-plugin/package.json index a4d3ce3a149445..b391ee35d16194 100644 --- a/packages/eslint-plugin/package.json +++ b/packages/eslint-plugin/package.json @@ -9,7 +9,8 @@ }, "license": "MIT", "scripts": { - "lint": "tsc --noEmit && eslint --ext .js --cache ." + "lint": "tsc --noEmit && eslint --ext .js --cache .", + "test": "yarn jest --passWithNoTests" }, "dependencies": { "@rnx-kit/eslint-plugin": "^0.2.5", @@ -26,6 +27,7 @@ "eslint-plugin-jsx-a11y": "^6.4.1", "eslint-plugin-react": "^7.24.0", "eslint-plugin-react-hooks": "^4.2.0", + "minimatch": "^3.0.4", "jju": "^1.4.0" }, "peerDependencies": { diff --git a/packages/eslint-plugin/src/configs/base.js b/packages/eslint-plugin/src/configs/base.js index d5de005e172002..54a1e8344d7f7f 100644 --- a/packages/eslint-plugin/src/configs/base.js +++ b/packages/eslint-plugin/src/configs/base.js @@ -1,9 +1,15 @@ // @ts-check const path = require('path'); +const configHelpers = require('../utils/configHelpers'); const { getNamingConventionRule } = require('../utils/configHelpers'); +/** @type {import("eslint").Linter.RulesRecord} */ +const typeAwareRules = { + '@fluentui/ban-context-export': ['error', { exclude: ['**/react-shared-contexts/**'] }], +}; + /** @type {import("eslint").Linter.Config} */ module.exports = { extends: [path.join(__dirname, 'core')], @@ -15,6 +21,8 @@ module.exports = { ...getNamingConventionRule(), }, overrides: [ + // Enable rules requiring type info only for appropriate files/circumstances + ...configHelpers.getTypeInfoRuleOverrides(typeAwareRules), { files: '**/src/index.{ts,tsx,js}', rules: { diff --git a/packages/eslint-plugin/src/index.js b/packages/eslint-plugin/src/index.js index 033c21c969dc30..3849051a743184 100644 --- a/packages/eslint-plugin/src/index.js +++ b/packages/eslint-plugin/src/index.js @@ -10,6 +10,7 @@ module.exports = { rules: { 'ban-imports': require('./rules/ban-imports'), + 'ban-context-export': require('./rules/ban-context-export'), 'deprecated-keyboard-event-props': require('./rules/deprecated-keyboard-event-props'), 'max-len': require('./rules/max-len'), 'no-global-react': require('./rules/no-global-react'), diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/context-selector/src/context.ts b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/context-selector/src/context.ts new file mode 100644 index 00000000000000..1be6d70682d0c6 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/context-selector/src/context.ts @@ -0,0 +1,4 @@ +// eslint-disable-next-line import/no-extraneous-dependencies +import { createContext } from '@fluentui/react-context-selector'; + +export const MyContext = createContext({}); diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/context-selector/src/index.ts b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/context-selector/src/index.ts new file mode 100644 index 00000000000000..f184fa5140be33 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/context-selector/src/index.ts @@ -0,0 +1,2 @@ +// This file needs to be in the file system for tests to run +// https://typescript-eslint.io/docs/development/custom-rules/#testing-typed-rules diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/context-selector/tsconfig.json b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/context-selector/tsconfig.json new file mode 100644 index 00000000000000..0703633adb8863 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/context-selector/tsconfig.json @@ -0,0 +1,11 @@ +{ + "extends": "../../../../../../../tsconfig.base.json", + "compilerOptions": { + "noEmit": false, + "lib": ["ES2019", "dom"], + "outDir": "dist", + "declaration": true, + "declarationDir": "dist/types", + "types": ["static-assets", "environment"] + } +} diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/exclude/special-path/src/index.ts b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/exclude/special-path/src/index.ts new file mode 100644 index 00000000000000..f184fa5140be33 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/exclude/special-path/src/index.ts @@ -0,0 +1,2 @@ +// This file needs to be in the file system for tests to run +// https://typescript-eslint.io/docs/development/custom-rules/#testing-typed-rules diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/exclude/tsconfig.json b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/exclude/tsconfig.json new file mode 100644 index 00000000000000..0703633adb8863 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/exclude/tsconfig.json @@ -0,0 +1,11 @@ +{ + "extends": "../../../../../../../tsconfig.base.json", + "compilerOptions": { + "noEmit": false, + "lib": ["ES2019", "dom"], + "outDir": "dist", + "declaration": true, + "declarationDir": "dist/types", + "types": ["static-assets", "environment"] + } +} diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/export-specifier/src/context.ts b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/export-specifier/src/context.ts new file mode 100644 index 00000000000000..fad576eb75a427 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/export-specifier/src/context.ts @@ -0,0 +1,4 @@ +// eslint-disable-next-line import/no-extraneous-dependencies +import * as React from 'react'; + +export const MyContext = React.createContext({}); diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/export-specifier/src/index.ts b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/export-specifier/src/index.ts new file mode 100644 index 00000000000000..f184fa5140be33 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/export-specifier/src/index.ts @@ -0,0 +1,2 @@ +// This file needs to be in the file system for tests to run +// https://typescript-eslint.io/docs/development/custom-rules/#testing-typed-rules diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/export-specifier/tsconfig.json b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/export-specifier/tsconfig.json new file mode 100644 index 00000000000000..0703633adb8863 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/export-specifier/tsconfig.json @@ -0,0 +1,11 @@ +{ + "extends": "../../../../../../../tsconfig.base.json", + "compilerOptions": { + "noEmit": false, + "lib": ["ES2019", "dom"], + "outDir": "dist", + "declaration": true, + "declarationDir": "dist/types", + "types": ["static-assets", "environment"] + } +} diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/internal-export/src/internal/context.ts b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/internal-export/src/internal/context.ts new file mode 100644 index 00000000000000..fad576eb75a427 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/internal-export/src/internal/context.ts @@ -0,0 +1,4 @@ +// eslint-disable-next-line import/no-extraneous-dependencies +import * as React from 'react'; + +export const MyContext = React.createContext({}); diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/internal-export/src/internal/index.ts b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/internal-export/src/internal/index.ts new file mode 100644 index 00000000000000..f184fa5140be33 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/internal-export/src/internal/index.ts @@ -0,0 +1,2 @@ +// This file needs to be in the file system for tests to run +// https://typescript-eslint.io/docs/development/custom-rules/#testing-typed-rules diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/internal-export/tsconfig.json b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/internal-export/tsconfig.json new file mode 100644 index 00000000000000..0703633adb8863 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/internal-export/tsconfig.json @@ -0,0 +1,11 @@ +{ + "extends": "../../../../../../../tsconfig.base.json", + "compilerOptions": { + "noEmit": false, + "lib": ["ES2019", "dom"], + "outDir": "dist", + "declaration": true, + "declarationDir": "dist/types", + "types": ["static-assets", "environment"] + } +} diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/named-export/src/index.ts b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/named-export/src/index.ts new file mode 100644 index 00000000000000..f184fa5140be33 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/named-export/src/index.ts @@ -0,0 +1,2 @@ +// This file needs to be in the file system for tests to run +// https://typescript-eslint.io/docs/development/custom-rules/#testing-typed-rules diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/named-export/tsconfig.json b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/named-export/tsconfig.json new file mode 100644 index 00000000000000..0703633adb8863 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/named-export/tsconfig.json @@ -0,0 +1,11 @@ +{ + "extends": "../../../../../../../tsconfig.base.json", + "compilerOptions": { + "noEmit": false, + "lib": ["ES2019", "dom"], + "outDir": "dist", + "declaration": true, + "declarationDir": "dist/types", + "types": ["static-assets", "environment"] + } +} diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/not-a-context/src/context.ts b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/not-a-context/src/context.ts new file mode 100644 index 00000000000000..5da144db61400b --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/not-a-context/src/context.ts @@ -0,0 +1,2 @@ +// eslint-disable-next-line import/no-extraneous-dependencies +export const MyContext = {}; diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/not-a-context/src/index.ts b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/not-a-context/src/index.ts new file mode 100644 index 00000000000000..f184fa5140be33 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/not-a-context/src/index.ts @@ -0,0 +1,2 @@ +// This file needs to be in the file system for tests to run +// https://typescript-eslint.io/docs/development/custom-rules/#testing-typed-rules diff --git a/packages/eslint-plugin/src/rules/ban-context-export/fixtures/not-a-context/tsconfig.json b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/not-a-context/tsconfig.json new file mode 100644 index 00000000000000..0703633adb8863 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/fixtures/not-a-context/tsconfig.json @@ -0,0 +1,11 @@ +{ + "extends": "../../../../../../../tsconfig.base.json", + "compilerOptions": { + "noEmit": false, + "lib": ["ES2019", "dom"], + "outDir": "dist", + "declaration": true, + "declarationDir": "dist/types", + "types": ["static-assets", "environment"] + } +} diff --git a/packages/eslint-plugin/src/rules/ban-context-export/index.js b/packages/eslint-plugin/src/rules/ban-context-export/index.js new file mode 100644 index 00000000000000..ab7b61bf9ce1c2 --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/index.js @@ -0,0 +1,135 @@ +// @ts-check +const { ESLintUtils, AST_NODE_TYPES } = require('@typescript-eslint/experimental-utils'); +const createRule = require('../../utils/createRule'); +const minimatch = require('minimatch'); + +/** @typedef { import('@typescript-eslint/experimental-utils').TSESTree.VariableDeclarator } VariableDeclarator*/ +/** @typedef { import('@typescript-eslint/experimental-utils').TSESTree.ExportSpecifier} ExportSpecifier */ +/** + * @typedef {{ + * exclude?: string[]; + * }} Options + */ + +/** @type {Options} */ +const defaultOptions = {}; + +module.exports = createRule({ + name: 'ban-context-export', + defaultOptions: [], + meta: { + schema: [ + { + type: 'object', + properties: { + exclude: { + type: 'array', + items: { + type: 'string', + }, + description: 'List of files to exclude', + }, + }, + additionalProperties: false, + }, + ], + type: 'problem', + docs: { + description: 'Ban export of React context or context selector objects', + category: 'Best Practices', + recommended: 'error', + }, + messages: { + nativeContext: '{{exportName}} should not be exported directly', + contextSelector: '{{exportName}} should not be exported directly', + }, + }, + create(context) { + const rawOptions = /** @type {Options[]} */ (context.options); + const { exclude = [] } = rawOptions.length ? rawOptions[0] : defaultOptions; + const { program, esTreeNodeToTSNodeMap } = ESLintUtils.getParserServices(context); + /** @type {import("typescript").TypeChecker | undefined} */ + let typeChecker; + + /** + * @param { ExportSpecifier | VariableDeclarator } node + * @param {string} exportName + */ + function checkContextType(node, exportName) { + const currentFileName = context.getFilename(); + if (exclude.some(pattern => minimatch(currentFileName, pattern))) { + return; + } + + const isTopLevelExport = currentFileName.endsWith('src/index.ts'); + if (!isTopLevelExport) { + return; + } + + if (!typeChecker) { + typeChecker = program.getTypeChecker(); + } + + const tsNode = esTreeNodeToTSNodeMap.get(node); + const typeNode = typeChecker.getTypeAtLocation(tsNode); + + // @fluentui/react-context-selector + if (typeNode.aliasSymbol?.name === 'Context' && typeNode.aliasSymbol.declarations?.length) { + const firstDeclaration = typeNode.aliasSymbol.declarations[0]; + const fileName = firstDeclaration.parent.getSourceFile().fileName; + if (fileName.includes('react-context-selector')) { + context.report({ + node, + messageId: 'contextSelector', + data: { + exportName, + filename: context.getFilename(), + }, + }); + } + } + + // Native react + if (typeNode.symbol?.name === 'Context' && typeNode.symbol.declarations?.length) { + const firstDeclaration = typeNode.symbol.declarations[0]; + const fileName = firstDeclaration.parent.getSourceFile().fileName; + if (/\/node_modules\/@types\/react\//.test(fileName)) { + context.report({ + node, + messageId: 'nativeContext', + data: { + exportName, + filename: context.getFilename(), + }, + }); + } + } + } + + return { + // eslint-disable-next-line @typescript-eslint/naming-convention + ExportNamedDeclaration(exportNamedDeclaration) { + if (exportNamedDeclaration.declaration?.type === AST_NODE_TYPES.VariableDeclaration) { + /** @type { import('@typescript-eslint/experimental-utils').TSESTree.VariableDeclaration } */ + const variableDeclaration = exportNamedDeclaration.declaration; + variableDeclaration.declarations.forEach(declaration => { + let identifierName = 'unknown'; + if (declaration.id.type === AST_NODE_TYPES.Identifier) { + /** @type { import('@typescript-eslint/experimental-utils').TSESTree.Identifier } */ + const identifier = declaration.id; + identifierName = identifier.name; + } + + checkContextType(declaration, identifierName); + }); + } + }, + // eslint-disable-next-line @typescript-eslint/naming-convention + ExportSpecifier(exportSpecifier) { + if (exportSpecifier.exported.name.includes('Context')) { + checkContextType(exportSpecifier, exportSpecifier.exported.name); + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin/src/rules/ban-context-export/index.test.js b/packages/eslint-plugin/src/rules/ban-context-export/index.test.js new file mode 100644 index 00000000000000..2088ed2c7f30bb --- /dev/null +++ b/packages/eslint-plugin/src/rules/ban-context-export/index.test.js @@ -0,0 +1,100 @@ +// @ts-check +const { ESLintUtils } = require('@typescript-eslint/experimental-utils'); +const path = require('path'); +const rule = require('./index'); + +const ruleTester = new ESLintUtils.RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + project: path.resolve(__dirname, './fixtures/ban-context-export/tsconfig.json'), + tsconfigRootDir: path.resolve(__dirname, './fixtures/ban-context-export'), + }, +}); + +/** + * @param {string} fixtureName + */ +function getParserOptions(fixtureName) { + return { + project: path.resolve(__dirname, `./fixtures/${fixtureName}/tsconfig.json`), + tsconfigRootDir: path.resolve(__dirname, `./fixtures/${fixtureName}`), + }; +} + +ruleTester.run('ban-context-export', rule, { + valid: [ + { + parserOptions: getParserOptions('internal-export'), + code: ` + export { MyContext } from './context' + `, + filename: 'src/internal/index.ts', + }, + { + parserOptions: getParserOptions('not-a-context'), + code: ` + export { MyContext } from './context' + `, + filename: 'src/index.ts', + }, + { + // No way to type generics with jsdoc + // @ts-ignore + options: [{ exclude: ['**/special-path/**/*'] }], + parserOptions: getParserOptions('exclude'), + code: ` + import * as React from 'react'; + export const MyContext = React.createContext({}); + `, + filename: 'special-path/src/index.ts', + }, + ], + invalid: [ + { + errors: [{ messageId: 'nativeContext' }], + parserOptions: getParserOptions('export-specifier'), + code: ` + export { MyContext } from './context' + `, + filename: 'src/index.ts', + }, + { + errors: [{ messageId: 'contextSelector' }], + parserOptions: getParserOptions('context-selector'), + code: ` + export { MyContext } from './context' + `, + filename: 'src/index.ts', + }, + { + errors: [{ messageId: 'nativeContext' }], + parserOptions: getParserOptions('named-export'), + code: ` + import * as React from 'react'; + export const MyContext = React.createContext({}); + `, + filename: 'src/index.ts', + }, + { + errors: [{ messageId: 'contextSelector' }], + parserOptions: getParserOptions('named-export'), + code: ` + import { createContext } from '@fluentui/react-context-selector'; + export const MyContext = createContext({}); + `, + filename: 'src/index.ts', + }, + { + errors: [{ messageId: 'nativeContext' }], + // No way to type generics with jsdoc + // @ts-ignore + options: [{ exclude: ['**/wrong-path/**/*'] }], + parserOptions: getParserOptions('exclude'), + code: ` + import * as React from 'react'; + export const MyContext = React.createContext({}); + `, + filename: 'special-path/src/index.ts', + }, + ], +}); diff --git a/packages/eslint-plugin/tsconfig.json b/packages/eslint-plugin/tsconfig.json index 880cdd2128c78c..1ad0e7809d02dd 100644 --- a/packages/eslint-plugin/tsconfig.json +++ b/packages/eslint-plugin/tsconfig.json @@ -10,5 +10,6 @@ "strict": true, "types": ["node"] }, - "include": ["src/**/*"] + "include": ["src/**/*"], + "exclude": ["**/fixtures/**/*"] } diff --git a/packages/react-components/react-context-selector/src/index.ts b/packages/react-components/react-context-selector/src/index.ts index 143a1c1951e627..bbc42ef78afce3 100644 --- a/packages/react-components/react-context-selector/src/index.ts +++ b/packages/react-components/react-context-selector/src/index.ts @@ -1,4 +1,5 @@ export { createContext } from './createContext'; export { useContextSelector } from './useContextSelector'; export { useHasParentContext } from './useHasParentContext'; +// eslint-disable-next-line @fluentui/ban-context-export export type { Context, ContextSelector, ContextValue, ContextValues, ContextVersion } from './types'; diff --git a/packages/react-components/react-utilities/src/index.ts b/packages/react-components/react-utilities/src/index.ts index d6cde7a98e2262..4749893eff58bb 100644 --- a/packages/react-components/react-utilities/src/index.ts +++ b/packages/react-components/react-utilities/src/index.ts @@ -46,6 +46,7 @@ export type { UseOnClickOrScrollOutsideOptions, } from './hooks/index'; +// eslint-disable-next-line @fluentui/ban-context-export export { canUseDOM, defaultSSRContextValue, useIsSSR, useSSRContext, SSRContext, SSRProvider } from './ssr/index'; export type { SSRContextValue } from './ssr/index';