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.

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

fix(eslint-plugin): make import/no-extraneous-dependencies work for react config #21924

Merged
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
6141a2d
fix(eslint-plugin): make import/no-extraneous-dependencies work for r…
Hotell Mar 2, 2022
fa7c232
Change files
Hotell Mar 3, 2022
c5f186f
chore: remove unused import-resolver-ts eslint plugin
Hotell Mar 3, 2022
14de40d
fix(react-conformance-griffel): add react-conformance missing dependency
Hotell Mar 4, 2022
c955bff
chore(pr-deploy-site): make lint work after import resolver changes
Hotell Mar 4, 2022
84ecd83
fix(react-avatar): fix missing dependencies
Hotell Mar 4, 2022
bc4d1d3
fix(react-accordion): fix missing dependencies
Hotell Mar 4, 2022
e29b1b1
fix(react-tabs): fix missing dependencies and lint warnings
Hotell Mar 4, 2022
6ca077f
fix(vr-tests-react-components): fix missing dependencies
Hotell Mar 4, 2022
a42389f
chore(cra-template): remove import/resolve from lint config
Hotell Mar 4, 2022
7708a55
chore(react-components): fix lint issues exposed by import/no-extrena…
Hotell Mar 4, 2022
ec845e9
feat(scripts): add file rename while copy behaviour to copy just-task
Hotell Mar 4, 2022
7c6f659
fixup! fix(eslint-plugin): make import/no-extraneous-dependencies wor…
Hotell Mar 8, 2022
2a48550
fixup! fixup! fix(eslint-plugin): make import/no-extraneous-dependenc…
Hotell Mar 8, 2022
02c338a
fixup! fixup! fixup! fix(eslint-plugin): make import/no-extraneous-de…
Hotell Mar 8, 2022
c6c42d9
fixup! fixup! fixup! fixup! fix(eslint-plugin): make import/no-extran…
Hotell Mar 8, 2022
305186e
fixup! fix(react-conformance-griffel): add react-conformance missing …
Hotell Mar 8, 2022
61aa885
Apply suggestions from code review
Hotell Mar 10, 2022
74d2ef8
fixup! Change files
Hotell Mar 10, 2022
0bcc3bc
chore: dedupe eslint-plugin-import
Hotell Mar 10, 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
7 changes: 0 additions & 7 deletions apps/pr-deploy-site/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,6 @@
{
"extends": ["plugin:@fluentui/eslint-plugin/node"],
"root": true,
"settings": {
"import/resolver": {
"typescript": {
"project": "../../tsconfig.json"
}
}
},
"overrides": [
{
// pr-deploy-site.js is run without transpiling in all browsers, which means it must use
Expand Down
6 changes: 3 additions & 3 deletions apps/pr-deploy-site/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@
"scripts": {
"clean": "just-scripts clean",
"generate:site": "just-scripts generate:site",
"lint": "eslint --ext .js,.ts --cache ."
"lint": "eslint --ext .js,.ts --cache .",
"type-check": "tsc -p ."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added type checking

},
"license": "MIT",
"devDependencies": {
"@fluentui/eslint-plugin": "*",
"@fluentui/scripts": "^1.0.0",
"@microsoft/eslint-plugin-sdl": "0.1.9"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied single version policy for devDeps

"@fluentui/scripts": "^1.0.0"
}
}
8 changes: 8 additions & 0 deletions apps/pr-deploy-site/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed to remove project config fallback to root tsconfig,json if tsconfig was missing in project.

this simplifies linting rules and makes our setup unified ( all packages have tsconfig and type-checking on CI )

"extends": "../../scripts/tsconfig.json",
"compilerOptions": {
"noEmit": true,
"allowJs": true
},
"include": ["pr-deploy-site.js", "just.config.ts"]
}
3 changes: 3 additions & 0 deletions apps/vr-tests-react-components/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,15 @@
"@fluentui/react-popover": "9.0.0-rc.5",
"@fluentui/react-positioning": "9.0.0-rc.5",
"@fluentui/react-provider": "9.0.0-rc.5",
"@fluentui/react-radio": "9.0.0-beta.2",
"@fluentui/react-shared-contexts": "9.0.0-rc.4",
"@fluentui/react-slider": "9.0.0-beta.10",
"@fluentui/react-switch": "9.0.0-rc.5",
"@fluentui/react-tabs": "9.0.0-beta.8",
"@fluentui/react-text": "9.0.0-rc.5",
"@fluentui/react-theme": "9.0.0-rc.4",
"@fluentui/react-tooltip": "9.0.0-rc.5",
"@fluentui/react-utilities": "9.0.0-rc.5",
"@fluentui/scripts": "^1.0.0",
"@griffel/react": "1.0.0",
"react": "16.14.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "chore(cra-template): remove import/resolve from lint config",
"packageName": "@fluentui/cra-template",
"email": "[email protected]",
"dependentChangeType": "none"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "fix(eslint-plugin): make import/no-extraneous-dependencies work for react config",
"packageName": "@fluentui/eslint-plugin",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "fixup! fix(react-accordion): fix missing dependencies",
"packageName": "@fluentui/react-accordion",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "fixup! fix(react-avatar): fix missing dependencies",
"packageName": "@fluentui/react-avatar",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "chore(react-components): fix lint issues exposed by import/no-extrenaus-dependency rule",
"packageName": "@fluentui/react-components",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "fix(react-conformance-griffel): add react-conformance missing dependency",
"packageName": "@fluentui/react-conformance-griffel",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "fix(react-provider): fix lint issues",
"packageName": "@fluentui/react-provider",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "fix(react-tabs): fix missing dependencies and lint warnings",
"packageName": "@fluentui/react-tabs",
"email": "[email protected]",
"dependentChangeType": "patch"
}
13 changes: 11 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
"@griffel/webpack-loader": "2.0.0",
"@linaria/babel-preset": "3.0.0-beta.14",
"@microsoft/api-extractor": "7.18.1",
"@microsoft/eslint-plugin-sdl": "0.1.9",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

single version policy

"@nrwl/cli": "13.8.1",
"@nrwl/devkit": "13.8.1",
"@nrwl/jest": "13.8.1",
Expand Down Expand Up @@ -179,10 +180,10 @@
"eslint": "7.25.0",
"eslint-config-airbnb": "18.2.1",
"eslint-config-prettier": "8.3.0",
"eslint-import-resolver-typescript": "2.4.0",
"eslint-import-resolver-typescript": "2.5.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bumped for bug fixes

"eslint-plugin-deprecation": "1.2.1",
"eslint-plugin-es": "4.1.0",
"eslint-plugin-import": "2.22.1",
"eslint-plugin-import": "2.25.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bumped to make properly resolve path aliases

"eslint-plugin-jest": "23.20.0",
"eslint-plugin-jsdoc": "^36.0.7",
"eslint-plugin-jsx-a11y": "6.4.1",
Expand Down Expand Up @@ -416,6 +417,14 @@
"ci-info",
"node-fetch"
]
},
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

version group needed, because we cannot specify * as proper dependency version

"packages": [
"@fluentui/react-conformance-griffel"
],
"dependencies": [
"@fluentui/react-conformance"
]
}
]
},
Expand Down
7 changes: 0 additions & 7 deletions packages/cra-template/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,6 @@
{
"extends": ["plugin:@fluentui/eslint-plugin/react"],
"root": true,
"settings": {
"import/resolver": {
"typescript": {
"project": "../../tsconfig.json"
}
}
},
"overrides": [
{
"files": ["template/**/*.{ts,tsx}"],
Expand Down
3 changes: 2 additions & 1 deletion packages/cra-template/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
},
"scripts": {
"lint": "eslint --ext .js,.ts,.tsx .",
"test": "node -r @fluentui/scripts/babel/register scripts/test.ts"
"test": "node -r @fluentui/scripts/babel/register scripts/test.ts",
"type-check": "tsc -p ."
},
"license": "MIT",
"files": [
Expand Down
8 changes: 8 additions & 0 deletions packages/cra-template/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"extends": "../../scripts/tsconfig.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed to remove project config fallback to root tsconfig,json if tsconfig was missing in project.

this simplifies linting rules and makes our setup unified ( all packages have tsconfig and type-checking on CI )

"compilerOptions": {
"noEmit": true
},
"include": ["scripts"],
"exclude": ["node_modules", "template"]
}
74 changes: 39 additions & 35 deletions packages/eslint-plugin/src/configs/react.js
Original file line number Diff line number Diff line change
@@ -1,41 +1,36 @@
// @ts-check
const path = require('path');
const configHelpers = require('../utils/configHelpers');

const gitRoot = configHelpers.findGitRoot();

/** @type {import("eslint").Linter.Config} */
const config = {
root: true,
extends: [
// Provides both rules and some parser options and other settings
'airbnb',
// add typescript support for import plugin - https://github.com/import-js/eslint-plugin-import/blob/main/config/typescript.js
'plugin:import/typescript',
// Extended configs are applied in order, so these configs that turn other rules off should come last
'prettier',
],
parser: '@typescript-eslint/parser',
plugins: [
'import',
'@fluentui',
'@rnx-kit',
'@typescript-eslint',
'deprecation',
'import',
'jest',
'jsdoc',
'jsx-a11y',
'react',
'react-hooks',
],
settings: {
// Some config suggestions copied from https://github.com/alexgorbatchev/eslint-import-resolver-typescript#configuration
'import/parsers': {
'@typescript-eslint/parser': ['.ts', '.tsx'],
},
'import/resolver': {
// @see https://github.com/alexgorbatchev/eslint-import-resolver-typescript#configuration
typescript: {
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 configuration was breaking import/no-extraneous-dependencies to work. now that typescript plugin is not being used at all and recommended configuration provided by import plugin is being used instead.

// always try to resolve types under `<root>@types` directory
alwaysTryTypes: true,
// NOTE: For packages without a tsconfig.json, override with "project": "../../tsconfig.json"
project: ['./tsconfig.json', path.join(gitRoot, 'tsconfig.json')],
project: './tsconfig.json',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now every package has tsconfig - no magic resolution needs to be used

},
},
jsdoc: {
Expand Down Expand Up @@ -102,7 +97,6 @@ const config = {
'dot-notation': 'error',
eqeqeq: ['error', 'always'],
'guard-for-in': 'error',
'import/no-extraneous-dependencies': ['error', { devDependencies: false }],
'jsx-a11y/tabindex-no-positive': 'error',
'no-alert': 'error',
'no-bitwise': 'error',
Expand Down Expand Up @@ -173,15 +167,7 @@ const config = {
'default-case': 'off',
'func-names': 'off',
'global-require': 'off',
'import/extensions': 'off',
'import/first': 'off',
'import/newline-after-import': 'off',
'import/no-duplicates': 'off', // mostly redundant with no-duplicate-imports
'import/no-dynamic-require': 'off',
'import/no-mutable-exports': 'off',
'import/no-unresolved': 'off',
'import/no-useless-path-segments': 'off',
'import/order': 'off',

'jsx-a11y/alt-text': 'off',
'jsx-a11y/anchor-is-valid': 'off',
'jsx-a11y/aria-activedescendant-has-tabindex': 'off',
Expand Down Expand Up @@ -256,17 +242,13 @@ const config = {
'no-restricted-syntax': 'off',

// permanently disable because we disagree with these rules
'import/prefer-default-export': 'off',
'no-await-in-loop': 'off', // contrary to rule docs, awaited things often are NOT parallelizable
'react/jsx-props-no-spreading': 'off',
'react/prop-types': 'off',

// permanently disable due to performance issues (using custom rule `@fluentui/max-len` instead)
'max-len': 'off',

// permanently disable due to being unnecessary or having limited benefit for TS
'import/export': 'off',

// permanently disable due to perf problems and limited benefit
// see here for perf testing (note that you must run eslint directly)
// https://eslint.org/docs/developer-guide/working-with-rules#per-rule-performance
Expand All @@ -277,24 +259,40 @@ const config = {
'react/no-unused-prop-types': 'off',
'react/prefer-es6-class': 'off',

'jsdoc/check-tag-names': [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no changes here - this occurs as diff because moving of lines....

'error',
{
// Allow TSDoc tags @remarks and @defaultValue
definedTags: ['remarks', 'defaultValue'],
},
],

/**
*
* import plugin rules
* @see https://github.com/import-js/eslint-plugin-import
Copy link
Contributor Author

@Hotell Hotell Mar 3, 2022

Choose a reason for hiding this comment

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

collocated all import plugins rules to one place so its easier to navigate/read

*/
'import/no-extraneous-dependencies': ['error', { devDependencies: false }],
'import/extensions': 'off',
'import/first': 'off',
'import/newline-after-import': 'off',
'import/no-duplicates': 'off', // mostly redundant with no-duplicate-imports
'import/no-dynamic-require': 'off',
'import/no-mutable-exports': 'off',
'import/no-unresolved': 'off',
'import/no-useless-path-segments': 'off',
'import/order': 'off',
'import/prefer-default-export': 'off',
// may cause perf problems per https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/FAQ.md#eslint-plugin-import
'import/no-cycle': 'off',
'import/no-deprecated': 'off',
'import/no-named-as-default': 'off',
'import/no-unused-modules': 'off',
// these ones aren't needed for TS and may cause perf problems
'import/default': 'off',
'import/named': 'off',
'import/namespace': 'off',
'import/no-named-as-default-member': 'off',

'jsdoc/check-tag-names': [
'error',
{
// Allow TSDoc tags @remarks and @defaultValue
definedTags: ['remarks', 'defaultValue'],
},
],
'import/export': 'off',
},
};

Expand Down Expand Up @@ -406,7 +404,13 @@ const getOverrides = () => [
{
files: [...configHelpers.devDependenciesFiles],
rules: {
'import/no-extraneous-dependencies': ['error', { packageDir: ['.', gitRoot] }],
// - turning off this rule for non production code
// - it doesn't work for our use cases as it should
// - we don't wanna specify monorepo packages as devDependencies in package.json
// if they are used in non production code. This rule is unable to handle this scenario.
// TODO:
// As a follow up we will introduce `@nrwl/nx/enforce-module-boundaries` with {"banTransitiveDependencies": true}
'import/no-extraneous-dependencies': ['off'],
},
},
];
Expand Down
1 change: 1 addition & 0 deletions packages/react-accordion/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"dependencies": {
"@fluentui/react-aria": "9.0.0-rc.5",
"@fluentui/react-context-selector": "9.0.0-rc.5",
"@fluentui/react-shared-contexts": "9.0.0-rc.4",
"@fluentui/react-icons": "^2.0.159-beta.10",
"@griffel/react": "1.0.0",
"@fluentui/react-tabster": "9.0.0-rc.5",
Expand Down
1 change: 1 addition & 0 deletions packages/react-avatar/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"@fluentui/react-badge": "9.0.0-rc.5",
"@fluentui/react-icons": "^2.0.159-beta.10",
"@fluentui/react-theme": "9.0.0-rc.4",
"@fluentui/react-shared-contexts": "9.0.0-rc.4",
"@fluentui/react-utilities": "9.0.0-rc.5",
"@griffel/react": "1.0.0",
"tslib": "^2.1.0"
Expand Down
2 changes: 1 addition & 1 deletion packages/react-components/config/pre-copy.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"copyTo": {
"unstable": ["./src/unstable/package.json"]
"unstable/package.json": "./src/unstable/package.json__tmpl__"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

leveraging new API : copy and rename to destination key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

package.json is using tmpl suffix used by nx, but there are no variables being interpolated within the file. the main purpose of this is so import/no-extreanous won't check this file ( which would trigger lint errors that are invalid )

}
}
4 changes: 0 additions & 4 deletions packages/react-components/src/unstable/tsconfig.json

This file was deleted.

9 changes: 7 additions & 2 deletions packages/react-conformance-griffel/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,15 @@
},
"devDependencies": {
"@fluentui/eslint-plugin": "*",
"@fluentui/scripts": "^1.0.0",
"@fluentui/react-conformance": "*"
"@fluentui/scripts": "^1.0.0"
},
"peerDependencies": {
"@types/react": ">=16.8.0 <18.0.0",
"@types/react-dom": ">=16.8.0 <18.0.0",
"typescript": "^4.3.0"
},
"dependencies": {
"@fluentui/react-conformance": "^0.10.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: create issue to get rid of v8 deps in v9 tree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"@griffel/react": "1.0.0",
"tslib": "^2.1.0"
},
Expand Down
1 change: 1 addition & 0 deletions packages/react-tabs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
},
"dependencies": {
"@griffel/react": "1.0.0",
"@fluentui/react-context-selector": "9.0.0-rc.5",
"@fluentui/react-tabster": "9.0.0-rc.5",
"@fluentui/react-theme": "9.0.0-rc.4",
"@fluentui/react-utilities": "9.0.0-rc.5",
Expand Down
Loading