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.

Sign up for GitHub

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: linting issues and rule changes #5681

Merged
merged 11 commits into from
Sep 23, 2024
Merged

fix: linting issues and rule changes #5681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ef43411
fix: lint config package updates
sriramveeraghanta Sep 19, 2024
5d14f3c
fix: tsconfig changes
sriramveeraghanta Sep 19, 2024
d4130d8
fix: lint config setup
sriramveeraghanta Sep 20, 2024
59b2fdb
fix: lint errors and adding new rules
sriramveeraghanta Sep 21, 2024
fec36cc
fix: lint errors
sriramveeraghanta Sep 21, 2024
45a2dc9
fix: ui and editor lints
sriramveeraghanta Sep 21, 2024
01ffa83
fix: build error
anmolsinghbhatia Sep 23, 2024
a71e182
fix: editor tsconfig
aaryan610 Sep 23, 2024
7d3689f
fix: merge conflicts resolved
sriramveeraghanta Sep 23, 2024
e1bca90
fix: lint errors
sriramveeraghanta Sep 23, 2024
8462fb7
fix: types fixes
sriramveeraghanta Sep 23, 2024
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
59 changes: 0 additions & 59 deletions .eslintrc-staged.js
View file
Open in desktop

This file was deleted.

10 changes: 0 additions & 10 deletions .eslintrc.js
View file
Open in desktop

This file was deleted.

Empty file removed .husky/pre-commit
View file
Open in desktop
Empty file.
3 changes: 0 additions & 3 deletions .lintstagedrc.json
View file
Open in desktop

This file was deleted.

52 changes: 4 additions & 48 deletions admin/.eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,8 @@
module.exports = {
root: true,
extends: ["custom"],
extends: ["@plane/eslint-config/next.js"],
parser: "@typescript-eslint/parser",
settings: {
"import/resolver": {
typescript: {},
node: {
moduleDirectory: ["node_modules", "."],
},
},
parserOptions: {
project: true,
},
rules: {
"import/order": [
"error",
{
groups: ["builtin", "external", "internal", "parent", "sibling",],
pathGroups: [
{
pattern: "react",
group: "external",
position: "before",
},
{
pattern: "lucide-react",
group: "external",
position: "after",
},
{
pattern: "@headlessui/**",
group: "external",
position: "after",
},
{
pattern: "@plane/**",
group: "external",
position: "after",
},
{
pattern: "@/**",
group: "internal",
}
],
pathGroupsExcludedImportTypes: ["builtin", "internal", "react"],
alphabetize: {
order: "asc",
caseInsensitive: true,
},
},
],
},
}
};
4 changes: 1 addition & 3 deletions admin/core/components/admin-sidebar/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ import { HelpSection, SidebarMenu, SidebarDropdown } from "@/components/admin-si
// hooks
import { useTheme } from "@/hooks/store";

export interface IInstanceSidebar {}

export const InstanceSidebar: FC<IInstanceSidebar> = observer(() => {
export const InstanceSidebar: FC = observer(() => {
// store
const { isSidebarCollapsed, toggleSidebar } = useTheme();

Expand Down
6 changes: 1 addition & 5 deletions admin/core/components/instance/instance-failure-view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,7 @@ import { Button } from "@plane/ui";
import InstanceFailureDarkImage from "@/public/instance/instance-failure-dark.svg";
import InstanceFailureImage from "@/public/instance/instance-failure.svg";

type InstanceFailureViewProps = {
// mutate: () => void;
};

export const InstanceFailureView: FC<InstanceFailureViewProps> = () => {
export const InstanceFailureView: FC = () => {
const { resolvedTheme } = useTheme();

const instanceImage = resolvedTheme === "dark" ? InstanceFailureDarkImage : InstanceFailureImage;
Expand Down
2 changes: 1 addition & 1 deletion admin/core/services/auth.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// helpers
import { API_BASE_URL } from "helpers/common.helper";
import { API_BASE_URL } from "@/helpers/common.helper";
// services
import { APIService } from "@/services/api.service";

Expand Down
4 changes: 2 additions & 2 deletions admin/core/services/user.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// helpers
import { API_BASE_URL } from "helpers/common.helper";
// types
import type { IUser } from "@plane/types";
// helpers
import { API_BASE_URL } from "@/helpers/common.helper";
// services
import { APIService } from "@/services/api.service";

Expand Down
9 changes: 5 additions & 4 deletions admin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
"build": "next build",
"preview": "next build && next start",
"start": "next start",
"lint": "next lint"
"lint": "eslint . --ext .ts,.tsx",
"lint:errors": "eslint . --ext .ts,.tsx --quiet"
},
"dependencies": {
"@headlessui/react": "^1.7.19",
Expand Down Expand Up @@ -43,9 +44,9 @@
"@types/react-dom": "^18.2.18",
"@types/uuid": "^9.0.8",
"@types/zxcvbn": "^4.4.4",
"eslint-config-custom": "*",
"@plane/eslint-config": "*",
"tailwind-config-custom": "*",
"tsconfig": "*",
"typescript": "5.4.5"
"@plane/typescript-config": "*",
"typescript": "5.3.3"
}
}
18 changes: 6 additions & 12 deletions admin/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,15 @@
{
"extends": "tsconfig/nextjs.json",
"include": ["next-env.d.ts", "**/*.ts", "**/*.tsx", ".next/types/**/*.ts"],
"exclude": ["node_modules"],
"extends": "@plane/typescript-config/nextjs.json",
"compilerOptions": {
"plugins": [{ "name": "next" }],
"baseUrl": ".",
"jsx": "preserve",
"esModuleInterop": true,
"paths": {
"@/*": ["core/*"],
"@/helpers/*": ["helpers/*"],
"@/public/*": ["public/*"],
"@/plane-admin/*": ["ce/*"]
},
"plugins": [
{
"name": "next"
}
]
}
}
},
"include": ["next-env.d.ts", "next.config.js", "**/*.ts", "**/*.tsx", ".next/types/**/*.ts"],
"exclude": ["node_modules"]
}
4 changes: 4 additions & 0 deletions live/.eslintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.turbo/*
out/*
dist/*
public/*
8 changes: 8 additions & 0 deletions live/.eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"root": true,
"extends": ["@plane/eslint-config/server.js"],
"parser": "@typescript-eslint/parser",
"parserOptions": {
"project": true
}
Comment on lines +5 to +7
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider specifying the tsconfig.json path

Setting "project": true in parserOptions is correct and allows the parser to use TypeScript's project configuration. This is necessary for TypeScript-specific rules that require type information.

However, for larger projects, this can significantly slow down linting. For better performance, consider explicitly specifying the path to your tsconfig.json file.

You could optimize this by changing the configuration to:

"parserOptions": {
  "project": "./tsconfig.json"
}

Replace "./tsconfig.json" with the actual path to your TypeScript configuration file if it's located elsewhere.

}
6 changes: 4 additions & 2 deletions live/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
"type": "module",
"scripts": {
"build": "babel src --out-dir dist --extensions \".ts,.js\"",
"start": "node dist/server.js"
"start": "node dist/server.js",
"lint": "eslint . --ext .ts,.tsx",
"lint:errors": "eslint . --ext .ts,.tsx --quiet"
},
"keywords": [],
"author": "",
Expand Down Expand Up @@ -55,6 +57,6 @@
"nodemon": "^3.1.0",
"ts-node": "^10.9.2",
"tsup": "^7.2.0",
"typescript": "5.4.5"
"typescript": "5.3.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Potential Compatibility Issue with TypeScript Downgrade

The TypeScript version has been downgraded to 5.3.3, but the web/helpers/filter.helper.ts file uses the satisfies keyword, a feature introduced in TypeScript 5.4. This mismatch may lead to compatibility issues or compilation errors.

  • web/helpers/filter.helper.ts uses the satisfies keyword, which is not supported in TypeScript 5.3.3.
Analysis chain

Please clarify the reason for downgrading TypeScript

The TypeScript version has been downgraded from 5.4.5 to 5.3.3. While there may be valid reasons for this change, it's important to ensure that it doesn't introduce compatibility issues or lose access to needed language features.

Could you please provide the rationale for this downgrade? Also, let's verify if this change affects any other parts of the project:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for TypeScript version references and potential incompatibilities

# Test: Search for TypeScript version references
echo "Searching for TypeScript version references:"
rg --type json '"typescript":\s*"[^"]*"' -g '!package.json'

# Test: Check for usage of TypeScript 5.4 features
echo "Checking for usage of TypeScript 5.4 features:"
rg --type typescript 'const\s+enum|satisfies\s+' -g '*.ts'

# Test: Check if there are any TypeScript errors after downgrade
echo "Checking for TypeScript errors after downgrade:"
npx tsc --noEmit

Length of output: 555

Script:

#!/bin/bash
# Description: Re-run TypeScript version checks with corrected commands

# Test: Search for TypeScript version references excluding package.json
echo "Searching for TypeScript version references:"
rg '"typescript":\s*"[^"]*"' -g '*.json' --ignore-file package.json

# Test: Check for usage of TypeScript 5.4 features by targeting .ts files
echo "Checking for usage of TypeScript 5.4 features:"
rg 'const\s+enum|satisfies\s+' -g '*.ts'

# Note: Skipping TypeScript compilation check as 'npx' is unavailable
echo "Skipping TypeScript compilation check due to environment limitations."

Length of output: 1565

}
}
3 changes: 1 addition & 2 deletions live/src/ce/types/common.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
// eslint-disable-next-line @typescript-eslint/ban-types
export type TAdditionalDocumentTypes = {}
export type TAdditionalDocumentTypes = {};
13 changes: 8 additions & 5 deletions live/src/core/extensions/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,16 @@ export const getExtensions: () => Promise<Extension[]> = async () => {
const documentType = params.get("documentType")?.toString() as
| TDocumentTypes
| undefined;

// TODO: Fix this lint error.
// eslint-disable-next-line no-async-promise-executor
Comment on lines +48 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor to address the linting issue properly

The added TODO comment and eslint-disable directive indicate a known linting issue with the use of an async executor in a Promise constructor. While the eslint-disable comment temporarily suppresses the error, it's better to refactor the code to address the underlying issue.

Consider refactoring the code to avoid using an async executor in the Promise constructor. Here's a suggested approach:

return Promise.resolve().then(async () => {
  try {
    let fetchedData = null;
    if (documentType === "project_page") {
      fetchedData = await fetchPageDescriptionBinary(
        params,
        pageId,
        cookie
      );
    } else {
      fetchedData = await fetchDocument({
        cookie,
        documentType,
        pageId,
        params,
      });
    }
    return fetchedData;
  } catch (error) {
    manualLogger.error("Error in fetching document", error);
    throw error; // Re-throw the error to be consistent with Promise rejection
  }
});

This refactoring wraps the async logic in a Promise.resolve().then() chain, which is a more idiomatic way to handle asynchronous operations in Promises. It also properly propagates errors by re-throwing them, ensuring that the Promise is rejected if an error occurs.

return new Promise(async (resolve) => {
try {
let fetchedData = null;
if (documentType === "project_page") {
fetchedData = await fetchPageDescriptionBinary(
params,
pageId,
cookie,
cookie
);
} else {
fetchedData = await fetchDocument({
Expand Down Expand Up @@ -83,6 +84,8 @@ export const getExtensions: () => Promise<Extension[]> = async () => {
| TDocumentTypes
| undefined;

// TODO: Fix this lint error.
// eslint-disable-next-line no-async-promise-executor
Comment on lines +87 to +88
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Refactor multiple async Promise executors and review similar patterns

Two instances of async Promise executors were found in live/src/core/extensions/index.ts:

  1. return new Promise(async (resolve) => {
  2. return new Promise(async () => {

Both instances use async functions within Promise constructors, which violates the no-async-promise-executor ESLint rule. Refactor these to eliminate the use of async executors and ensure proper error handling.

Analysis chain

Refactor to address the linting issue and review similar patterns

Similar to the previous instance, this code segment uses an async executor in a Promise constructor, which is flagged by the linter. The added TODO comment and eslint-disable directive are temporary solutions that should be replaced with a proper refactor.

  1. Refactor this code segment similarly to the previous suggestion:
return Promise.resolve().then(async () => {
  try {
    if (documentType === "project_page") {
      await updatePageDescription(params, pageId, state, cookie);
    } else {
      await updateDocument({
        cookie,
        documentType,
        pageId,
        params,
        updatedDescription: state,
      });
    }
  } catch (error) {
    manualLogger.error("Error in updating document:", error);
    throw error; // Re-throw the error to be consistent with Promise rejection
  }
});
  1. Conduct a broader review of the codebase for similar patterns. This recurring issue suggests that there might be other instances of this anti-pattern that need addressing.

To help identify other instances of this pattern in the codebase, you can run the following command:

This will help locate other occurrences of this pattern that may need refactoring.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for async Promise executors in TypeScript files
rg --type typescript 'new Promise\(async' -C 3

Length of output: 83

Script:

#!/bin/bash
# Search for async Promise executors in TypeScript files
rg --type ts 'new Promise\(async' -C 3

Length of output: 1076

return new Promise(async () => {
try {
if (documentType === "project_page") {
Expand Down Expand Up @@ -121,7 +124,7 @@ export const getExtensions: () => Promise<Extension[]> = async () => {
}
manualLogger.warn(
`Redis Client wasn't able to connect, continuing without Redis (you won't be able to sync data between multiple plane live servers)`,
error,
error
);
reject(error);
});
Expand All @@ -135,12 +138,12 @@ export const getExtensions: () => Promise<Extension[]> = async () => {
} catch (error) {
manualLogger.warn(
`Redis Client wasn't able to connect, continuing without Redis (you won't be able to sync data between multiple plane live servers)`,
error,
error
);
}
} else {
manualLogger.warn(
"Redis URL is not set, continuing without Redis (you won't be able to sync data between multiple plane live servers)",
"Redis URL is not set, continuing without Redis (you won't be able to sync data between multiple plane live servers)"
);
}

Expand Down
2 changes: 1 addition & 1 deletion live/src/core/helpers/error-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { ErrorRequestHandler } from "express";
import { manualLogger } from "@/core/helpers/logger.js";

export const errorHandler: ErrorRequestHandler = (err, _req, res, _next) => {
export const errorHandler: ErrorRequestHandler = (err, _req, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Caution: Removing _next parameter may break error handling middleware chain

The removal of the _next parameter from the errorHandler function could potentially break the error handling middleware chain in Express. The ErrorRequestHandler type typically expects four parameters: (err, req, res, next).

Consider keeping the _next parameter even if unused:

export const errorHandler: ErrorRequestHandler = (err, _req, res, _next) => {
  // ... existing code ...
};

This ensures compatibility with Express's error handling middleware expectations.

// Log the error
manualLogger.error(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

Enhance error logging with additional context

While the current implementation logs the error, it could be improved by including more context.

Consider enhancing the error logging:

manualLogger.error('An error occurred', {
  error: err,
  stack: err.stack,
  url: _req.url,
  method: _req.method
});

This provides more information for debugging purposes.


Expand Down
31 changes: 7 additions & 24 deletions live/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1,26 @@
{
"extends": "tsconfig/base.json",
"extends": "@plane/typescript-config/base.json",
"compilerOptions": {
"module": "NodeNext",
"moduleResolution": "NodeNext",

"lib": [
"ES2015"
],

"lib": ["ES2015"],
"outDir": "./dist",
"rootDir": "./src",
"rootDir": ".",
"baseUrl": ".",

"paths": {
"@/*": [
"./src/*"
],
"@/plane-live/*": [
"./src/ce/*"
]
"@/*": ["./src/*"],
"@/plane-live/*": ["./src/ce/*"]
},

"removeComments": true,
"esModuleInterop": true,
"skipLibCheck": true,
"sourceMap": true,
"inlineSources": true,

// Set `sourceRoot` to "/" to strip the build path prefix
// from generated source code references.
// This improves issue grouping in Sentry.
"sourceRoot": "/"
},
"include": [
"src/**/*.ts"
],
"exclude": [
"./dist",
"./build",
"./node_modules"
]
"include": ["src/**/*.ts", "tsup.config.ts"],
"exclude": ["./dist", "./build", "./node_modules"]
}
Loading
Loading