From e7b7d28732e108ba0ca698d93d2f19d4583fbc3d Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Sat, 11 Nov 2023 11:19:01 +0800 Subject: [PATCH 1/6] Fail CI on dependency cycles --- .eslintrc.js | 2 +- .github/workflows/ci.yml | 16 +-------------- package-lock.json | 44 +++++++++++++++++++++++++--------------- package.json | 7 ++++--- src/globals.d.ts | 4 ---- tsconfig.json | 5 +---- 6 files changed, 35 insertions(+), 43 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 76fd0e0b6b..52ec60aa2c 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -93,7 +93,7 @@ module.exports = { "@typescript-eslint/restrict-template-expressions": "warn", // Enabled for the IDE, but it's disabled in the `lint` script - "import/no-cycle": "warn", + "import/no-cycle": "error", }, overrides: [ { diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bfb2d4b3e3..48e874e2de 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -152,21 +152,7 @@ jobs: node-version-file: ".nvmrc" cache: npm - run: npm ci - - run: npm run lint -- --quiet - - circular-deps: - runs-on: ubuntu-latest - - steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 - with: - node-version-file: ".nvmrc" - cache: npm - # Fixed version to avoid random breaks. Not in package.json due to #1084 - - run: npm install --global madge@5.0.1 - - run: npm run madge:circular - name: Detect circular dependencies + - run: npm run lint:full # https://pre-commit.com/#usage-in-continuous-integration prettier: diff --git a/package-lock.json b/package-lock.json index 465be5c017..fc9f3258c9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -193,6 +193,7 @@ "@types/json-stringify-safe": "^5.0.2", "@types/lodash": "^4.14.200", "@types/mark.js": "^8.11.10", + "@types/marked": "^6.0.0", "@types/mustache": "^4.2.4", "@types/nunjucks": "^3.2.5", "@types/object-hash": "^2.1.1", @@ -8374,6 +8375,16 @@ "@types/jquery": "*" } }, + "node_modules/@types/marked": { + "version": "6.0.0", + "resolved": "https://registry.npmjs.org/@types/marked/-/marked-6.0.0.tgz", + "integrity": "sha512-jmjpa4BwUsmhxcfsgUit/7A9KbrC48Q0q8KvnY107ogcjGgTFDlIL3RpihNpx2Mu1hM4mdFQjoVc4O6JoGKHsA==", + "deprecated": "This is a stub types definition. marked provides its own type definitions, so you do not need this installed.", + "dev": true, + "dependencies": { + "marked": "*" + } + }, "node_modules/@types/mdx": { "version": "2.0.9", "resolved": "https://registry.npmjs.org/@types/mdx/-/mdx-2.0.9.tgz", @@ -14762,22 +14773,6 @@ "node": ">=8" } }, - "node_modules/eslint/node_modules/optionator": { - "version": "0.9.3", - "integrity": "sha512-JjCoypp+jKn1ttEFExxhetCKeJt9zhAgAve5FXHixTvFDW/5aEktX9bufBKLRRMdU7bNtpLfcGu94B3cdEJgjg==", - "dev": true, - "dependencies": { - "@aashutoshrathi/word-wrap": "^1.2.3", - "deep-is": "^0.1.3", - "fast-levenshtein": "^2.0.6", - "levn": "^0.4.1", - "prelude-ls": "^1.2.1", - "type-check": "^0.4.0" - }, - "engines": { - "node": ">= 0.8.0" - } - }, "node_modules/eslint/node_modules/supports-color": { "version": "7.2.0", "integrity": "sha512-qpCAvRl9stuOHveKsn7HncJRvv501qIacKzQlO/+Lwxc9+0q2wLyv4Dfvt80/DPn2pqOBsJdDiogXGR9+OvwRw==", @@ -21857,6 +21852,23 @@ "opener": "bin/opener-bin.js" } }, + "node_modules/optionator": { + "version": "0.9.3", + "resolved": "https://registry.npmjs.org/optionator/-/optionator-0.9.3.tgz", + "integrity": "sha512-JjCoypp+jKn1ttEFExxhetCKeJt9zhAgAve5FXHixTvFDW/5aEktX9bufBKLRRMdU7bNtpLfcGu94B3cdEJgjg==", + "dev": true, + "dependencies": { + "@aashutoshrathi/word-wrap": "^1.2.3", + "deep-is": "^0.1.3", + "fast-levenshtein": "^2.0.6", + "levn": "^0.4.1", + "prelude-ls": "^1.2.1", + "type-check": "^0.4.0" + }, + "engines": { + "node": ">= 0.8.0" + } + }, "node_modules/ora": { "version": "5.4.1", "integrity": "sha512-5b6Y85tPxZZ7QytO+BQzysW31HJku27cRIlkbAXaNx+BdcVi+LlRFmVXzeF6a7JCwJpyw5c4b+YSVImQIrBpuQ==", diff --git a/package.json b/package.json index 3787492bb4..88df883f92 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,8 @@ "scripts": { "test": "TZ=UTC jest", "test:watch": "TZ=UTC jest --watchAll", - "lint": "eslint src --ext js,jsx,ts,tsx --quiet --report-unused-disable-directives --rule '{\"import/no-cycle\": \"off\"}'", + "lint": "npm run lint:full -- --rule '{\"import/no-cycle\": \"off\"}'", + "lint:full": "eslint src --ext js,jsx,ts,tsx --quiet --report-unused-disable-directives", "fix": "npm run lint -- --fix", "watch": "concurrently npm:watch:webpack 'npm:watch:*(!webpack) -- --preserveWatchOutput' -r", "watch:webpack": "ENV_FILE='.env.development' webpack --mode development --watch", @@ -18,8 +19,7 @@ "build:scripts": "webpack --config scripts/webpack.scripts.js", "generate:headers": "NODE_OPTIONS=--stack-trace-limit=100 node scripts/bin/headers.js", "storybook": "storybook dev -p 6006 -s public", - "build-storybook": "storybook build", - "madge:circular": "madge --extensions js,ts,tsx --circular src/** --ts-config tsconfig.json" + "build-storybook": "storybook build" }, "engine-strict": true, "engines": { @@ -214,6 +214,7 @@ "@types/json-stringify-safe": "^5.0.2", "@types/lodash": "^4.14.200", "@types/mark.js": "^8.11.10", + "@types/marked": "^6.0.0", "@types/mustache": "^4.2.4", "@types/nunjucks": "^3.2.5", "@types/object-hash": "^2.1.1", diff --git a/src/globals.d.ts b/src/globals.d.ts index 3a92632337..1c72d2c519 100644 --- a/src/globals.d.ts +++ b/src/globals.d.ts @@ -88,10 +88,6 @@ declare module "generate-schema" { const json: (title: string, obj: unknown) => UnknownObject; } -// The package breaks Madge, so we have to include a patch in tsconfig, which breaks the @types package. -// In the end, the types aren't even used. -declare module "marked"; - // No types available declare module "@pixiebrix/jq-web"; declare module "canvas-confetti"; diff --git a/tsconfig.json b/tsconfig.json index 3f781aa9cb..8ee418898e 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -22,10 +22,7 @@ "@schemas/*": ["schemas/*"], "@microsoft/applicationinsights-web/*": [ "src/contrib/uipath/quietLogger/*" - ], - - // Help Madge resolve this dependency - "marked": ["node_modules/marked/lib/marked.esm.js"] + ] }, "plugins": [{ "name": "typescript-plugin-css-modules" }] }, From e04593cb00a93f606c363fdfd3f4d85dc5cb5095 Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Sat, 11 Nov 2023 11:26:11 +0800 Subject: [PATCH 2/6] Add demo dependency cycle (should break CI) --- src/bricks/errors.ts | 3 +++ src/errors/errorHelpers.ts | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/src/bricks/errors.ts b/src/bricks/errors.ts index 3d8a3b6444..b68530e5ac 100644 --- a/src/bricks/errors.ts +++ b/src/bricks/errors.ts @@ -25,6 +25,9 @@ import { type RegistryId } from "@/types/registryTypes"; import { type Schema } from "@/types/schemaTypes"; import { type BrickArgs, type BrickArgsContext } from "@/types/runtimeTypes"; import { isObject } from "@/utils/objectUtils"; +import { logTest } from "@/errors/errorHelpers"; + +setTimeout(logTest, 100); export class PipelineConfigurationError extends BusinessError { override name = "PipelineConfigurationError"; diff --git a/src/errors/errorHelpers.ts b/src/errors/errorHelpers.ts index a671b3a27a..f9bbac8093 100644 --- a/src/errors/errorHelpers.ts +++ b/src/errors/errorHelpers.ts @@ -39,6 +39,10 @@ import { const DEFAULT_ERROR_MESSAGE = "Unknown error"; +export function logTest() { + console.log("test", isSchemaValidationError); +} + /** * Errors to ignore unless they've caused extension point install or brick execution to fail. * From 5bc95ff2262363507fb01bf5ac8a808f9541233b Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Sat, 11 Nov 2023 14:37:00 +0800 Subject: [PATCH 3/6] Revert "Add demo dependency cycle (should break CI)" This reverts commit e04593cb00a93f606c363fdfd3f4d85dc5cb5095. --- src/bricks/errors.ts | 3 --- src/errors/errorHelpers.ts | 4 ---- 2 files changed, 7 deletions(-) diff --git a/src/bricks/errors.ts b/src/bricks/errors.ts index b68530e5ac..3d8a3b6444 100644 --- a/src/bricks/errors.ts +++ b/src/bricks/errors.ts @@ -25,9 +25,6 @@ import { type RegistryId } from "@/types/registryTypes"; import { type Schema } from "@/types/schemaTypes"; import { type BrickArgs, type BrickArgsContext } from "@/types/runtimeTypes"; import { isObject } from "@/utils/objectUtils"; -import { logTest } from "@/errors/errorHelpers"; - -setTimeout(logTest, 100); export class PipelineConfigurationError extends BusinessError { override name = "PipelineConfigurationError"; diff --git a/src/errors/errorHelpers.ts b/src/errors/errorHelpers.ts index f9bbac8093..a671b3a27a 100644 --- a/src/errors/errorHelpers.ts +++ b/src/errors/errorHelpers.ts @@ -39,10 +39,6 @@ import { const DEFAULT_ERROR_MESSAGE = "Unknown error"; -export function logTest() { - console.log("test", isSchemaValidationError); -} - /** * Errors to ignore unless they've caused extension point install or brick execution to fail. * From ed072bd9f32f04f896f58c179421be964a8365df Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Sat, 11 Nov 2023 14:38:17 +0800 Subject: [PATCH 4/6] Update comment --- .eslintrc.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.eslintrc.js b/.eslintrc.js index 52ec60aa2c..3f1981ea1e 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -92,7 +92,7 @@ module.exports = { // Rules that depend on https://github.com/pixiebrix/pixiebrix-extension/issues/775 "@typescript-eslint/restrict-template-expressions": "warn", - // Enabled for the IDE, but it's disabled in the `lint` script + // Enabled on CI and in the IDE, but it's disabled in the local `lint` script because it's excessively slow "import/no-cycle": "error", }, overrides: [ From e9e4418245789d75b5d8b906f751965b344039b1 Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Sun, 12 Nov 2023 00:21:13 +0800 Subject: [PATCH 5/6] Move to shared config --- .eslintrc.js | 3 --- package-lock.json | 8 ++++---- package.json | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 3f1981ea1e..94a7b43e33 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -91,9 +91,6 @@ module.exports = { // Rules that depend on https://github.com/pixiebrix/pixiebrix-extension/issues/775 "@typescript-eslint/restrict-template-expressions": "warn", - - // Enabled on CI and in the IDE, but it's disabled in the local `lint` script because it's excessively slow - "import/no-cycle": "error", }, overrides: [ { diff --git a/package-lock.json b/package-lock.json index fc9f3258c9..1674e36c38 100644 --- a/package-lock.json +++ b/package-lock.json @@ -228,7 +228,7 @@ "css-minimizer-webpack-plugin": "^3.4.1", "dotenv": "^16.3.1", "eslint": "^8.53.0", - "eslint-config-pixiebrix": "^0.31.0", + "eslint-config-pixiebrix": "^0.32.0", "eslint-plugin-local-rules": "^2.0.0", "fake-indexeddb": "^5.0.1", "identity-obj-proxy": "^3.0.0", @@ -13709,9 +13709,9 @@ } }, "node_modules/eslint-config-pixiebrix": { - "version": "0.31.0", - "resolved": "https://registry.npmjs.org/eslint-config-pixiebrix/-/eslint-config-pixiebrix-0.31.0.tgz", - "integrity": "sha512-UIUXVZhz+HmfhdaNYQSUHzXnp0ctMnqdpGNLl/3aUibxE9E14TocDH+CCI9rYXbYEq/mRTvnDKy4HPUbiDCofQ==", + "version": "0.32.0", + "resolved": "https://registry.npmjs.org/eslint-config-pixiebrix/-/eslint-config-pixiebrix-0.32.0.tgz", + "integrity": "sha512-jJZMFjXq3VtW0CeiizR8qL47IIG2TTrVAg5R8d6XoQYurT+c8n2dDOT1jNZuK6wSdGvRdJcJz+I4N77yViHqRA==", "dev": true, "dependencies": { "@typescript-eslint/eslint-plugin": "^6.7.0", diff --git a/package.json b/package.json index 88df883f92..512c5ca560 100644 --- a/package.json +++ b/package.json @@ -249,7 +249,7 @@ "css-minimizer-webpack-plugin": "^3.4.1", "dotenv": "^16.3.1", "eslint": "^8.53.0", - "eslint-config-pixiebrix": "^0.31.0", + "eslint-config-pixiebrix": "^0.32.0", "eslint-plugin-local-rules": "^2.0.0", "fake-indexeddb": "^5.0.1", "identity-obj-proxy": "^3.0.0", From 28054d96f4aeecde6875d6cd1e18935ca1356f3a Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Sun, 12 Nov 2023 00:35:35 +0800 Subject: [PATCH 6/6] Restore lint:fast script --- .eslintrc.js | 9 +++++++++ package.json | 1 + 2 files changed, 10 insertions(+) diff --git a/.eslintrc.js b/.eslintrc.js index 94a7b43e33..e588e0b238 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -126,3 +126,12 @@ module.exports = { }, ], }; + +// `npm run lint:fast` will skip the (slow) import/* rules +// Useful if you're trying to iterate fixes over other rules +if (process.env.ESLINT_NO_IMPORTS) { + const importRules = Object.keys(require("eslint-plugin-import").rules); + for (const ruleName of importRules) { + module.exports.rules[`import/${ruleName}`] = "off"; + } +} diff --git a/package.json b/package.json index 512c5ca560..a4158f0fcb 100644 --- a/package.json +++ b/package.json @@ -6,6 +6,7 @@ "test": "TZ=UTC jest", "test:watch": "TZ=UTC jest --watchAll", "lint": "npm run lint:full -- --rule '{\"import/no-cycle\": \"off\"}'", + "lint:fast": "ESLINT_NO_IMPORTS=1 eslint src --ext js,jsx,ts,tsx --quiet", "lint:full": "eslint src --ext js,jsx,ts,tsx --quiet --report-unused-disable-directives", "fix": "npm run lint -- --fix", "watch": "concurrently npm:watch:webpack 'npm:watch:*(!webpack) -- --preserveWatchOutput' -r",