From 9ef070d33a486a696b425b53935bed8a213af727 Mon Sep 17 00:00:00 2001 From: Ivan Akulov Date: Fri, 9 Jun 2023 11:44:58 +0200 Subject: [PATCH] fix(eslint): fix direct `remixicon` imports in `packages/design-system/*` (#24010) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description This PR: - moves the icon-related ESLint rules from to `eslintrc.base.json` – to make them apply to all packages - fixes direct `remixicon` imports surfaced by the change #### Type of change - Bug fix (non-breaking change which fixes an issue) ## Testing #### How Has This Been Tested? - [ ] Manual – smoke-tested that the dashboard and the editor open correctly - [ ] Jest - [ ] Cypress #### Test Plan > Add Testsmith test cases links that relate to this PR > > #### Issues raised during DP testing > Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR) > > > ## Checklist: #### Dev activity - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Test-plan-implementation#speedbreaker-features-to-consider-for-every-change) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans/_edit#areas-of-interest) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed --- app/client/.eslintrc.base.json | 22 ++++++++++++++- app/client/.eslintrc.js | 28 ++++++++----------- app/client/package.json | 1 + .../src/components/Checkbox/Checkbox.tsx | 21 ++++++++++++-- .../src/components/Field/ErrorText.tsx | 10 ++++++- .../headless/src/components/Field/Label.tsx | 16 ++++++++++- .../src/components/Button/Button.test.tsx | 16 ++++++++++- .../src/components/Checkbox/Checkbox.test.tsx | 16 ++++++++++- .../src/components/Spinner/index.styled.tsx | 24 +++++++++++++++- .../Evaluation/fns/__tests__/interval.test.ts | 4 +-- app/client/yarn.lock | 1 + 11 files changed, 132 insertions(+), 27 deletions(-) diff --git a/app/client/.eslintrc.base.json b/app/client/.eslintrc.base.json index 65a20d8f27..6826984d32 100644 --- a/app/client/.eslintrc.base.json +++ b/app/client/.eslintrc.base.json @@ -54,7 +54,27 @@ { "caseSensitive": false } ], "no-console": "warn", - "no-debugger": "warn" + "no-debugger": "warn", + "@typescript-eslint/no-restricted-imports": [ + "error", + { + "patterns": [ + { + "group": ["@blueprintjs/core/lib/esnext/*"], + "message": "Reason: @blueprintjs/core has both lib/esnext and lib/esm directories which export the same components. To avoid duplicating components in the bundle, please import only from the lib/esm directory." + }, + { + "group": ["*.svg"], + "importNames": ["ReactComponent"], + "message": "Reason: Please don’t import SVG icons statically. (They won’t always be needed, but they *will* always be present in the bundle and will increase the bundle size.) Instead, please either import them as SVG paths (e.g. import starIconUrl from './star.svg'), or use the importSvg wrapper from design-system-old (e.g. const StarIcon = importSvg(() => import('./star.svg')))." + }, + { + "group": ["remixicon-react/*"], + "message": "Reason: Please don’t import Remix icons statically. (They won’t always be needed, but they *will* always be present in the bundle and will increase the bundle size.) Instead, please use the importRemixIcon wrapper from design-system-old (e.g. const StarIcon = importRemixIcon(() => import('remixicon-react/Star')))." + } + ] + } + ] }, "settings": { "import/resolver": { diff --git a/app/client/.eslintrc.js b/app/client/.eslintrc.js index 7db2dd3ebe..45bc21c710 100644 --- a/app/client/.eslintrc.js +++ b/app/client/.eslintrc.js @@ -1,5 +1,15 @@ // The `@type` comment improves auto-completion for VS Code users: https://github.com/appsmithorg/appsmith/pull/21602#discussion_r1144528505 /** @type {import('eslint').Linter.Config} */ +const fs = require("fs"); +const path = require("path"); +const JSON5 = require("json5"); + +const baseEslintConfig = JSON5.parse( + fs.readFileSync(path.join(__dirname, "./.eslintrc.base.json"), "utf8"), +); +const baseNoRestrictedImports = + baseEslintConfig.rules["@typescript-eslint/no-restricted-imports"][1]; + const eslintConfig = { extends: ["./.eslintrc.base.json"], rules: { @@ -10,6 +20,7 @@ const eslintConfig = { "error", { paths: [ + ...(baseNoRestrictedImports.paths ?? []), { name: "codemirror", message: @@ -26,22 +37,7 @@ const eslintConfig = { }, ], patterns: [ - { - group: ["@blueprintjs/core/lib/esnext/*"], - message: - "Reason: @blueprintjs/core has both lib/esnext and lib/esm directories which export the same components. To avoid duplicating components in the bundle, please import only from the lib/esm directory.", - }, - { - group: ["*.svg"], - importNames: ["ReactComponent"], - message: - "Reason: Please don’t import SVG icons statically. (They won’t always be needed, but they *will* always be present in the bundle and will increase the bundle size.) Instead, please either import them as SVG paths (e.g. import starIconUrl from './star.svg'), or use the importSvg wrapper from design-system-old (e.g. const StarIcon = importSvg(() => import('./star.svg'))).", - }, - { - group: ["remixicon-react/*"], - message: - "Reason: Please don’t import Remix icons statically. (They won’t always be needed, but they *will* always be present in the bundle and will increase the bundle size.) Instead, please use the importRemixIcon wrapper from design-system-old (e.g. const StarIcon = importRemixIcon(() => import('remixicon-react/Star'))).", - }, + ...(baseNoRestrictedImports.patterns ?? []), { group: ["**/ce/*"], message: "Reason: Please use @appsmith import instead.", diff --git a/app/client/package.json b/app/client/package.json index 880f437fca..fe38d8c380 100644 --- a/app/client/package.json +++ b/app/client/package.json @@ -306,6 +306,7 @@ "jest-canvas-mock": "^2.3.1", "jest-environment-jsdom": "^27.4.1", "jest-styled-components": "^7.0.8", + "json5": "^2.2.3", "lint-staged": "^13.2.0", "msw": "^0.28.0", "plop": "^3.1.1", diff --git a/app/client/packages/design-system/headless/src/components/Checkbox/Checkbox.tsx b/app/client/packages/design-system/headless/src/components/Checkbox/Checkbox.tsx index ae7ab969fc..7bb5a7e064 100644 --- a/app/client/packages/design-system/headless/src/components/Checkbox/Checkbox.tsx +++ b/app/client/packages/design-system/headless/src/components/Checkbox/Checkbox.tsx @@ -1,18 +1,33 @@ import { mergeProps } from "@react-aria/utils"; import { useFocusRing } from "@react-aria/focus"; import { useHover } from "@react-aria/interactions"; -import CheckIcon from "remixicon-react/CheckLineIcon"; import { useToggleState } from "@react-stately/toggle"; import { useFocusableRef } from "@react-spectrum/utils"; -import SubtractIcon from "remixicon-react/SubtractLineIcon"; import React, { forwardRef, useContext, useRef } from "react"; import { useVisuallyHidden } from "@react-aria/visually-hidden"; import type { SpectrumCheckboxProps } from "@react-types/checkbox"; import type { FocusableRef, StyleProps } from "@react-types/shared"; import { useCheckbox, useCheckboxGroupItem } from "@react-aria/checkbox"; - import { CheckboxGroupContext } from "./context"; +// Adapted from remixicon-react/CheckLineIcon (https://github.com/Remix-Design/RemixIcon/blob/f88a51b6402562c6c2465f61a3e845115992e4c6/icons/System/check-line.svg) +const CheckIcon = ({ size }: { size: number }) => { + return ( + + + + ); +}; + +// Adapted from remixicon-react/SubtractLineIcon (https://github.com/Remix-Design/RemixIcon/blob/f88a51b6402562c6c2465f61a3e845115992e4c6/icons/System/subtract-line.svg) +const SubtractIcon = ({ size }: { size: number }) => { + return ( + + + + ); +}; + export interface CheckboxProps extends Omit { icon?: React.ReactNode; diff --git a/app/client/packages/design-system/headless/src/components/Field/ErrorText.tsx b/app/client/packages/design-system/headless/src/components/Field/ErrorText.tsx index 34d89074ba..27212c2028 100644 --- a/app/client/packages/design-system/headless/src/components/Field/ErrorText.tsx +++ b/app/client/packages/design-system/headless/src/components/Field/ErrorText.tsx @@ -1,9 +1,17 @@ import React, { forwardRef } from "react"; import type { HTMLAttributes } from "react"; import { useDOMRef } from "@react-spectrum/utils"; -import AlertIcon from "remixicon-react/AlertFillIcon"; import type { DOMRef, SpectrumHelpTextProps } from "@react-types/shared"; +// Adapted from remixicon-react/AlertFillIcon (https://github.com/Remix-Design/RemixIcon/blob/f88a51b6402562c6c2465f61a3e845115992e4c6/icons/System/alert-fill.svg) +const AlertIcon = () => { + return ( + + + + ); +}; + interface HelpTextProps extends SpectrumHelpTextProps { errorMessageProps?: HTMLAttributes; } diff --git a/app/client/packages/design-system/headless/src/components/Field/Label.tsx b/app/client/packages/design-system/headless/src/components/Field/Label.tsx index 06b7573c99..fc0b359531 100644 --- a/app/client/packages/design-system/headless/src/components/Field/Label.tsx +++ b/app/client/packages/design-system/headless/src/components/Field/Label.tsx @@ -2,9 +2,23 @@ import React, { forwardRef } from "react"; import { useDOMRef } from "@react-spectrum/utils"; import type { DOMRef } from "@react-types/shared"; import { filterDOMProps } from "@react-aria/utils"; -import AsteriskIcon from "remixicon-react/AsteriskIcon"; import type { SpectrumLabelProps } from "@react-types/label"; +// Adapted from remixicon-react/AsteriskIcon (https://github.com/Remix-Design/RemixIcon/blob/f88a51b6402562c6c2465f61a3e845115992e4c6/icons/Editor/asterisk.svg) +const AsteriskIcon = (props: { [key: string]: string | undefined }) => { + return ( + + + + ); +}; + export interface LabelProps extends SpectrumLabelProps { isEmphasized?: boolean; } diff --git a/app/client/packages/design-system/widgets/src/components/Button/Button.test.tsx b/app/client/packages/design-system/widgets/src/components/Button/Button.test.tsx index 558aa676aa..3eba6bca2b 100644 --- a/app/client/packages/design-system/widgets/src/components/Button/Button.test.tsx +++ b/app/client/packages/design-system/widgets/src/components/Button/Button.test.tsx @@ -2,10 +2,24 @@ import React from "react"; import "@testing-library/jest-dom"; import { Icon } from "@design-system/headless"; import { render, screen } from "@testing-library/react"; -import EmotionHappyLineIcon from "remixicon-react/EmotionHappyLineIcon"; import { Button } from "./"; +// Adapted from remixicon-react/EmotionHappyLineIcon (https://github.com/Remix-Design/RemixIcon/blob/f88a51b6402562c6c2465f61a3e845115992e4c6/icons/User%20%26%20Faces/emotion-happy-line.svg) +const EmotionHappyLineIcon = ({ ...props }: Record) => { + return ( + + + + ); +}; + describe("@design-system/widgets/Button", () => { it("renders children when passed", () => { render(); diff --git a/app/client/packages/design-system/widgets/src/components/Checkbox/Checkbox.test.tsx b/app/client/packages/design-system/widgets/src/components/Checkbox/Checkbox.test.tsx index 97b965fc9b..91f6b3d874 100644 --- a/app/client/packages/design-system/widgets/src/components/Checkbox/Checkbox.test.tsx +++ b/app/client/packages/design-system/widgets/src/components/Checkbox/Checkbox.test.tsx @@ -3,10 +3,24 @@ import "@testing-library/jest-dom"; import { Icon } from "@design-system/headless"; import { render, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import EmotionHappyLineIcon from "remixicon-react/EmotionHappyLineIcon"; import { Checkbox } from "./Checkbox"; +// Adapted from remixicon-react/EmotionHappyLineIcon (https://github.com/Remix-Design/RemixIcon/blob/f88a51b6402562c6c2465f61a3e845115992e4c6/icons/User%20%26%20Faces/emotion-happy-line.svg) +const EmotionHappyLineIcon = ({ ...props }: Record) => { + return ( + + + + ); +}; + describe("@design-system/widgets/Checkbox", () => { const onChangeSpy = jest.fn(); diff --git a/app/client/packages/design-system/widgets/src/components/Spinner/index.styled.tsx b/app/client/packages/design-system/widgets/src/components/Spinner/index.styled.tsx index 59c2a0bc20..b7fbe84fe6 100644 --- a/app/client/packages/design-system/widgets/src/components/Spinner/index.styled.tsx +++ b/app/client/packages/design-system/widgets/src/components/Spinner/index.styled.tsx @@ -1,5 +1,27 @@ +import React from "react"; import styled from "styled-components"; -import LoaderIcon from "remixicon-react/Loader2FillIcon"; + +// Adapted from remixicon-react/Loader2FillIcon (https://github.com/Remix-Design/RemixIcon/blob/f88a51b6402562c6c2465f61a3e845115992e4c6/icons/System/loader-2-fill.svg) +function LoaderIcon({ + className, + ...props +}: { + className?: string; + [key: string]: any; +}) { + return ( + + + + ); +} export const StyledSpinner = styled(LoaderIcon)` animation: spin 1s linear infinite; diff --git a/app/client/src/workers/Evaluation/fns/__tests__/interval.test.ts b/app/client/src/workers/Evaluation/fns/__tests__/interval.test.ts index d474dd928e..42963e86d4 100644 --- a/app/client/src/workers/Evaluation/fns/__tests__/interval.test.ts +++ b/app/client/src/workers/Evaluation/fns/__tests__/interval.test.ts @@ -80,7 +80,7 @@ describe("Tests for interval functions", () => { // eslint-disable-next-line jest/no-disabled-tests it.skip("Callback should have access to outer scope variables", async () => { const stalker = jest.fn(); - function test() { + function runTest() { let count = 0; const interval = evalContext.setInterval(() => { count++; @@ -88,7 +88,7 @@ describe("Tests for interval functions", () => { }, 100); return interval; } - const interval = test(); + const interval = runTest(); await new Promise((resolve) => setTimeout(resolve, 300)); clearInterval(interval); expect(stalker).toBeCalledTimes(2); diff --git a/app/client/yarn.lock b/app/client/yarn.lock index 79dcc13bbc..36c5184222 100644 --- a/app/client/yarn.lock +++ b/app/client/yarn.lock @@ -9679,6 +9679,7 @@ __metadata: js-regex-pl: ^1.0.1 js-sha256: ^0.9.0 jshint: ^2.13.4 + json5: ^2.2.3 klona: ^2.0.5 libphonenumber-js: ^1.9.44 linkedom: ^0.14.20