From d00d5d1f6cc986d794482d548220c886c9582ec2 Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Thu, 19 Sep 2024 14:05:49 +0530 Subject: [PATCH 01/14] upadte: added telemety for jshint --- .../src/plugins/Linting/utils/getLintingErrors.ts | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/app/client/src/plugins/Linting/utils/getLintingErrors.ts b/app/client/src/plugins/Linting/utils/getLintingErrors.ts index 74b0b60ea62..2a681a875c4 100644 --- a/app/client/src/plugins/Linting/utils/getLintingErrors.ts +++ b/app/client/src/plugins/Linting/utils/getLintingErrors.ts @@ -36,6 +36,8 @@ import setters from "workers/Evaluation/setters"; import { isMemberExpressionNode } from "@shared/ast/src"; import { generate } from "astring"; import getInvalidModuleInputsError from "ee/plugins/Linting/utils/getInvalidModuleInputsError"; +import { startAndEndSpanForFn } from "UITelemetry/generateTraces"; +import { objectKeys } from "@appsmith/utils"; const EvaluationScriptPositions: Record = {}; @@ -67,7 +69,7 @@ function generateLintingGlobalData(data: Record) { libAccessors.forEach((accessor) => (globalData[accessor] = true)); // Add all supported web apis - Object.keys(SUPPORTED_WEB_APIS).forEach( + objectKeys(SUPPORTED_WEB_APIS).forEach( (apiName) => (globalData[apiName] = true), ); @@ -195,7 +197,16 @@ export default function getLintingErrors({ const lintingGlobalData = generateLintingGlobalData(data); const lintingOptions = lintOptions(lintingGlobalData); - jshint(script, lintingOptions); + startAndEndSpanForFn( + "Linter", + // adding some metrics to compare the performance changes with eslint + { + linter: "JSHint", + linesOfCodeLinted: originalBinding.split("\n").length, + codeSizeInChars: originalBinding.length, + }, + () => jshint(script, lintingOptions), + ); const sanitizedJSHintErrors = sanitizeJSHintErrors(jshint.errors, scriptPos); const jshintErrors: LintError[] = sanitizedJSHintErrors.map((lintError) => convertJsHintErrorToAppsmithLintError( From a3a0612cfbdff0e21faacbe318c9b769260aa4e3 Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Thu, 19 Sep 2024 18:55:47 +0530 Subject: [PATCH 02/14] update: types for each linter handler function to add telemetry --- .../plugins/Linting/handlers/lintService.ts | 8 ++++++-- .../Linting/handlers/setupLinkingWorkerEnv.ts | 5 ++++- .../handlers/updateJSLibraryGlobals.ts | 6 ++++-- .../src/plugins/Linting/linters/worker.ts | 20 +++++++++++++++---- 4 files changed, 30 insertions(+), 9 deletions(-) diff --git a/app/client/src/plugins/Linting/handlers/lintService.ts b/app/client/src/plugins/Linting/handlers/lintService.ts index b907745e172..5735c3e4aa1 100644 --- a/app/client/src/plugins/Linting/handlers/lintService.ts +++ b/app/client/src/plugins/Linting/handlers/lintService.ts @@ -9,6 +9,7 @@ import { PathUtils } from "plugins/Linting/utils/pathUtils"; import { extractReferencesFromPath } from "ee/plugins/Linting/utils/getEntityDependencies"; import { groupDifferencesByType } from "plugins/Linting/utils/groupDifferencesByType"; import type { + LintRequest, LintTreeRequestPayload, LintTreeResponse, } from "plugins/Linting/types"; @@ -39,7 +40,8 @@ class LintService { } } - lintTree = (payload: LintTreeRequestPayload) => { + lintTree = (lintRequest: LintRequest) => { + const { data: payload, webworkerTelemetry } = lintRequest; const { cloudHosting, configTree, @@ -78,6 +80,7 @@ class LintService { errors: {}, lintedJSPaths: [], jsPropertiesState, + webworkerTelemetry, }; try { @@ -87,12 +90,13 @@ class LintService { jsPropertiesState, cloudHosting, asyncJSFunctionsInDataFields, - + webworkerTelemetry, configTree, }); lintTreeResponse.errors = lintErrors; lintTreeResponse.lintedJSPaths = lintedJSPaths; + lintTreeResponse.webworkerTelemetry = webworkerTelemetry; } catch (e) {} return lintTreeResponse; diff --git a/app/client/src/plugins/Linting/handlers/setupLinkingWorkerEnv.ts b/app/client/src/plugins/Linting/handlers/setupLinkingWorkerEnv.ts index 99f5577f95b..90b20ee2a37 100644 --- a/app/client/src/plugins/Linting/handlers/setupLinkingWorkerEnv.ts +++ b/app/client/src/plugins/Linting/handlers/setupLinkingWorkerEnv.ts @@ -1,6 +1,9 @@ import type { FeatureFlags } from "ee/entities/FeatureFlag"; import { WorkerEnv } from "workers/Evaluation/handlers/workerEnv"; +import type { LintRequest } from "../types"; -export const setupLintingWorkerEnv = (featureFlags: FeatureFlags) => { +export const setupLintingWorkerEnv = ({ + data: featureFlags, +}: LintRequest) => { WorkerEnv.setFeatureFlags(featureFlags); }; diff --git a/app/client/src/plugins/Linting/handlers/updateJSLibraryGlobals.ts b/app/client/src/plugins/Linting/handlers/updateJSLibraryGlobals.ts index e89cc369dd2..8be516e6e9d 100644 --- a/app/client/src/plugins/Linting/handlers/updateJSLibraryGlobals.ts +++ b/app/client/src/plugins/Linting/handlers/updateJSLibraryGlobals.ts @@ -1,9 +1,11 @@ -import type { updateJSLibraryProps } from "plugins/Linting/types"; +import type { LintRequest, updateJSLibraryProps } from "plugins/Linting/types"; import { isEqual } from "lodash"; import { JSLibraries, JSLibraryAccessor } from "workers/common/JSLibrary"; import { resetJSLibraries } from "workers/common/JSLibrary/resetJSLibraries"; -export function updateJSLibraryGlobals(data: updateJSLibraryProps) { +export function updateJSLibraryGlobals({ + data, +}: LintRequest) { const { add, libs } = data; if (add) { diff --git a/app/client/src/plugins/Linting/linters/worker.ts b/app/client/src/plugins/Linting/linters/worker.ts index 8704b1f2474..2445fd1caec 100644 --- a/app/client/src/plugins/Linting/linters/worker.ts +++ b/app/client/src/plugins/Linting/linters/worker.ts @@ -1,17 +1,29 @@ import type { TMessage } from "utils/MessageUtil"; import { MessageType } from "utils/MessageUtil"; import { WorkerMessenger } from "workers/Evaluation/fns/utils/Messenger"; -import type { LintRequest } from "../types"; +import type { + LintRequest, + LintTreeRequestPayload, + updateJSLibraryProps, +} from "../types"; import { handlerMap } from "../handlers"; +import type { FeatureFlags } from "ee/entities/FeatureFlag"; -export function messageListener(e: MessageEvent>) { +// The messageListener can have either of these three types +type LinterFunctionPayload = LintTreeRequestPayload & + FeatureFlags & + updateJSLibraryProps; + +export function messageListener( + e: MessageEvent>>, +) { const { messageType } = e.data; if (messageType !== MessageType.REQUEST) return; const startTime = Date.now(); const { body, messageId } = e.data; - const { data, method } = body; + const { method } = body; if (!method) return; @@ -19,7 +31,7 @@ export function messageListener(e: MessageEvent>) { if (typeof messageHandler !== "function") return; - const responseData = messageHandler(data); + const responseData = messageHandler(body); if (!responseData) return; From 15cccc079bba891f0bcf07f6d5024d20c118c369 Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Thu, 19 Sep 2024 18:56:34 +0530 Subject: [PATCH 03/14] update: type definitions for linter functions --- app/client/src/plugins/Linting/types.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/app/client/src/plugins/Linting/types.ts b/app/client/src/plugins/Linting/types.ts index 0a53ea3a3d4..a0de1b5a390 100644 --- a/app/client/src/plugins/Linting/types.ts +++ b/app/client/src/plugins/Linting/types.ts @@ -11,6 +11,10 @@ import type { import type { DependencyMap } from "utils/DynamicBindingUtils"; import type { TJSPropertiesState } from "workers/Evaluation/JSObject/jsPropertiesState"; import type { JSLibrary } from "workers/common/JSLibrary"; +import type { WebworkerSpanData } from "UITelemetry/generateWebWorkerTraces"; +import type { SpanAttributes } from "UITelemetry/generateTraces"; + +export type WebworkerTelemetryAttribute = WebworkerSpanData | SpanAttributes; export enum LINT_WORKER_ACTIONS { LINT_TREE = "LINT_TREE", @@ -21,6 +25,7 @@ export interface LintTreeResponse { errors: LintErrorsStore; lintedJSPaths: string[]; jsPropertiesState: TJSPropertiesState; + webworkerTelemetry: Record; } export interface LintTreeRequestPayload { @@ -30,11 +35,10 @@ export interface LintTreeRequestPayload { forceLinting?: boolean; } -export interface LintRequest { - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - data: any; +export interface LintRequest { + data: T; method: LINT_WORKER_ACTIONS; + webworkerTelemetry: Record; } export interface LintTreeSagaRequestData { @@ -46,12 +50,14 @@ export interface lintTriggerPathProps { userScript: string; entity: DataTreeEntity; globalData: ReturnType; + webworkerTelemetry: Record; } export interface lintBindingPathProps { dynamicBinding: string; entity: DataTreeEntity; fullPropertyPath: string; globalData: ReturnType; + webworkerTelemetry: Record; } export interface getLintingErrorsProps { script: string; @@ -62,6 +68,7 @@ export interface getLintingErrorsProps { options?: { isJsObject: boolean; }; + webworkerTelemetry: Record; } export interface getLintErrorsFromTreeProps { @@ -71,6 +78,7 @@ export interface getLintErrorsFromTreeProps { cloudHosting: boolean; asyncJSFunctionsInDataFields: DependencyMap; configTree: ConfigTree; + webworkerTelemetry: Record; } export interface getLintErrorsFromTreeResponse { From 36c925b03413149ce955c8a0cdc8fab93b705e0d Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Thu, 19 Sep 2024 18:57:32 +0530 Subject: [PATCH 04/14] update: passing down telemetry function to linter functions --- app/client/src/plugins/Linting/lintTree.ts | 5 +++++ app/client/src/plugins/Linting/utils/lintBindingPath.ts | 2 ++ app/client/src/plugins/Linting/utils/lintJSObjectBody.ts | 3 +++ app/client/src/plugins/Linting/utils/lintJSObjectProperty.ts | 3 +++ app/client/src/plugins/Linting/utils/lintJSProperty.ts | 3 +++ app/client/src/plugins/Linting/utils/lintTriggerPath.ts | 2 ++ 6 files changed, 18 insertions(+) diff --git a/app/client/src/plugins/Linting/lintTree.ts b/app/client/src/plugins/Linting/lintTree.ts index 1afbcefb306..5f1a37d3785 100644 --- a/app/client/src/plugins/Linting/lintTree.ts +++ b/app/client/src/plugins/Linting/lintTree.ts @@ -21,6 +21,7 @@ export function getLintErrorsFromTree({ jsPropertiesState, pathsToLint, unEvalTree, + webworkerTelemetry, }: getLintErrorsFromTreeProps): getLintErrorsFromTreeResponse { const lintTreeErrors: LintErrorsStore = {}; const lintedJSPaths = new Set(); @@ -47,6 +48,7 @@ export function getLintErrorsFromTree({ entity, fullPropertyPath: bindingPath, globalData: globalData.getGlobalData(false), + webworkerTelemetry, }); set(lintTreeErrors, `["${bindingPath}"]`, lintErrors); @@ -67,6 +69,7 @@ export function getLintErrorsFromTree({ userScript: unEvalPropertyValue, entity, globalData: globalData.getGlobalData(true), + webworkerTelemetry, }); set(lintTreeErrors, `["${triggerPath}"]`, lintErrors); @@ -87,6 +90,7 @@ export function getLintErrorsFromTree({ const jsObjectBodyLintErrors = lintJSObjectBody( jsObjectName, globalData.getGlobalData(true), + webworkerTelemetry, ); set(lintTreeErrors, jsObjectBodyPath, jsObjectBodyLintErrors); @@ -96,6 +100,7 @@ export function getLintErrorsFromTree({ jsObjectPath, jsObjectState, asyncJSFunctionsInDataFields, + webworkerTelemetry, ); const currentLintErrorsInBody = get( lintTreeErrors, diff --git a/app/client/src/plugins/Linting/utils/lintBindingPath.ts b/app/client/src/plugins/Linting/utils/lintBindingPath.ts index 590e97ffbcf..6a16e8f668e 100644 --- a/app/client/src/plugins/Linting/utils/lintBindingPath.ts +++ b/app/client/src/plugins/Linting/utils/lintBindingPath.ts @@ -11,6 +11,7 @@ export default function lintBindingPath({ entity, fullPropertyPath, globalData, + webworkerTelemetry, }: lintBindingPathProps) { let lintErrors: LintError[] = []; const { propertyPath } = getEntityNameAndPropertyPath(fullPropertyPath); @@ -37,6 +38,7 @@ export default function lintBindingPath({ data: globalData, originalBinding, scriptType, + webworkerTelemetry, }); lintErrors = lintErrors.concat(lintErrorsFromSnippet); diff --git a/app/client/src/plugins/Linting/utils/lintJSObjectBody.ts b/app/client/src/plugins/Linting/utils/lintJSObjectBody.ts index f3b1be6426d..6c9087b3741 100644 --- a/app/client/src/plugins/Linting/utils/lintJSObjectBody.ts +++ b/app/client/src/plugins/Linting/utils/lintJSObjectBody.ts @@ -11,10 +11,12 @@ import { import { Severity } from "entities/AppsmithConsole"; import { getJSToLint } from "./getJSToLint"; import getLintingErrors from "./getLintingErrors"; +import type { WebworkerTelemetryAttribute } from "../types"; export default function lintJSObjectBody( jsObjectName: string, globalData: DataTree, + webworkerTelemetry: Record, ): LintError[] { const jsObject = globalData[jsObjectName]; const rawJSObjectbody = (jsObject as unknown as JSActionEntity).body; @@ -50,5 +52,6 @@ export default function lintJSObjectBody( data: globalData, originalBinding: jsbodyToLint, scriptType, + webworkerTelemetry, }); } diff --git a/app/client/src/plugins/Linting/utils/lintJSObjectProperty.ts b/app/client/src/plugins/Linting/utils/lintJSObjectProperty.ts index 692a1171b74..5c836e49413 100644 --- a/app/client/src/plugins/Linting/utils/lintJSObjectProperty.ts +++ b/app/client/src/plugins/Linting/utils/lintJSObjectProperty.ts @@ -11,11 +11,13 @@ import { globalData } from "../globalData"; import getLintSeverity from "./getLintSeverity"; import lintJSProperty from "./lintJSProperty"; import { isEmpty } from "lodash"; +import type { WebworkerTelemetryAttribute } from "../types"; export default function lintJSObjectProperty( jsPropertyFullName: string, jsObjectState: Record, asyncJSFunctionsInDataFields: DependencyMap, + webworkerTelemetry: Record, ) { let lintErrors: LintError[] = []; const { propertyPath: jsPropertyName } = @@ -29,6 +31,7 @@ export default function lintJSObjectProperty( jsPropertyFullName, jsPropertyState, globalData.getGlobalData(!isAsyncJSFunctionBoundToSyncField), + webworkerTelemetry, ); lintErrors = lintErrors.concat(jsPropertyLintErrors); diff --git a/app/client/src/plugins/Linting/utils/lintJSProperty.ts b/app/client/src/plugins/Linting/utils/lintJSProperty.ts index 331b8335ee3..3d87c1ff314 100644 --- a/app/client/src/plugins/Linting/utils/lintJSProperty.ts +++ b/app/client/src/plugins/Linting/utils/lintJSProperty.ts @@ -8,11 +8,13 @@ import { } from "workers/Evaluation/evaluate"; import type { TJSpropertyState } from "workers/Evaluation/JSObject/jsPropertiesState"; import getLintingErrors from "./getLintingErrors"; +import type { WebworkerTelemetryAttribute } from "../types"; export default function lintJSProperty( jsPropertyFullName: string, jsPropertyState: TJSpropertyState, globalData: DataTree, + webworkerTelemetry: Record, ): LintError[] { if (isNil(jsPropertyState)) { return []; @@ -29,6 +31,7 @@ export default function lintJSProperty( originalBinding: jsPropertyState.value, scriptType, options: { isJsObject: true }, + webworkerTelemetry, }); const refinedErrors = propLintErrors.map((lintError) => { return { diff --git a/app/client/src/plugins/Linting/utils/lintTriggerPath.ts b/app/client/src/plugins/Linting/utils/lintTriggerPath.ts index d31c6a1c7ea..01dd8ff8eae 100644 --- a/app/client/src/plugins/Linting/utils/lintTriggerPath.ts +++ b/app/client/src/plugins/Linting/utils/lintTriggerPath.ts @@ -10,6 +10,7 @@ export default function lintTriggerPath({ entity, globalData, userScript, + webworkerTelemetry, }: lintTriggerPathProps) { const { jsSnippets } = getDynamicBindings(userScript, entity); const script = getScriptToEval(jsSnippets[0], EvaluationScriptType.TRIGGERS); @@ -19,5 +20,6 @@ export default function lintTriggerPath({ data: globalData, originalBinding: jsSnippets[0], scriptType: EvaluationScriptType.TRIGGERS, + webworkerTelemetry, }); } From 0da7c9d57dda7b3ddbc6bad0d78217080fb2bf6c Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Thu, 19 Sep 2024 18:58:34 +0530 Subject: [PATCH 05/14] update: method to profile linter func --- .../plugins/Linting/tests/getLintingErrors.test.ts | 11 +++++++++++ .../src/plugins/Linting/utils/getLintingErrors.ts | 6 ++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts b/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts index d70ba9bcf1d..c92385bfd5e 100644 --- a/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts +++ b/app/client/src/plugins/Linting/tests/getLintingErrors.test.ts @@ -2,6 +2,8 @@ import { getScriptType } from "workers/Evaluation/evaluate"; import { CustomLintErrorCode } from "../constants"; import getLintingErrors from "../utils/getLintingErrors"; +const webworkerTelemetry = {}; + describe("getLintingErrors", () => { describe("1. Verify lint errors are not shown for supported window APIs", () => { const data = {}; @@ -17,6 +19,7 @@ describe("getLintingErrors", () => { originalBinding, script, scriptType, + webworkerTelemetry, }); expect(Array.isArray(lintErrors)).toBe(true); @@ -29,6 +32,7 @@ describe("getLintingErrors", () => { const scriptType = getScriptType(false, true); const lintErrors = getLintingErrors({ + webworkerTelemetry, data, originalBinding, script, @@ -45,6 +49,7 @@ describe("getLintingErrors", () => { const scriptType = getScriptType(false, true); const lintErrors = getLintingErrors({ + webworkerTelemetry, data, originalBinding, script, @@ -65,6 +70,7 @@ describe("getLintingErrors", () => { const scriptType = getScriptType(false, true); const lintErrors = getLintingErrors({ + webworkerTelemetry, data, originalBinding, script, @@ -80,6 +86,7 @@ describe("getLintingErrors", () => { const scriptType = getScriptType(false, true); const lintErrors = getLintingErrors({ + webworkerTelemetry, data, originalBinding, script, @@ -95,6 +102,7 @@ describe("getLintingErrors", () => { const scriptType = getScriptType(false, true); const lintErrors = getLintingErrors({ + webworkerTelemetry, data, originalBinding, script, @@ -128,6 +136,7 @@ describe("getLintingErrors", () => { const scriptType = getScriptType(false, false); const lintErrors = getLintingErrors({ + webworkerTelemetry, data, options, originalBinding, @@ -150,6 +159,7 @@ describe("getLintingErrors", () => { const scriptType = getScriptType(false, false); const lintErrors = getLintingErrors({ + webworkerTelemetry, data, options, originalBinding, @@ -172,6 +182,7 @@ describe("getLintingErrors", () => { const scriptType = getScriptType(false, false); const lintErrors = getLintingErrors({ + webworkerTelemetry, data, options, originalBinding, diff --git a/app/client/src/plugins/Linting/utils/getLintingErrors.ts b/app/client/src/plugins/Linting/utils/getLintingErrors.ts index 2a681a875c4..eccbe7d0afc 100644 --- a/app/client/src/plugins/Linting/utils/getLintingErrors.ts +++ b/app/client/src/plugins/Linting/utils/getLintingErrors.ts @@ -36,8 +36,8 @@ import setters from "workers/Evaluation/setters"; import { isMemberExpressionNode } from "@shared/ast/src"; import { generate } from "astring"; import getInvalidModuleInputsError from "ee/plugins/Linting/utils/getInvalidModuleInputsError"; -import { startAndEndSpanForFn } from "UITelemetry/generateTraces"; import { objectKeys } from "@appsmith/utils"; +import { profileFn } from "UITelemetry/generateWebWorkerTraces"; const EvaluationScriptPositions: Record = {}; @@ -192,12 +192,13 @@ export default function getLintingErrors({ originalBinding, script, scriptType, + webworkerTelemetry, }: getLintingErrorsProps): LintError[] { const scriptPos = getEvaluationScriptPosition(scriptType); const lintingGlobalData = generateLintingGlobalData(data); const lintingOptions = lintOptions(lintingGlobalData); - startAndEndSpanForFn( + profileFn( "Linter", // adding some metrics to compare the performance changes with eslint { @@ -205,6 +206,7 @@ export default function getLintingErrors({ linesOfCodeLinted: originalBinding.split("\n").length, codeSizeInChars: originalBinding.length, }, + webworkerTelemetry, () => jshint(script, lintingOptions), ); const sanitizedJSHintErrors = sanitizeJSHintErrors(jshint.errors, scriptPos); From da5b190689303622bcf1d489520d61eacbd9dbbd Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Fri, 20 Sep 2024 01:52:42 +0530 Subject: [PATCH 06/14] create: alternate way to calculate linter length --- .../editorComponents/CodeEditor/lintHelpers.ts | 16 ++++++++++------ app/client/src/utils/DynamicBindingUtils.ts | 1 + 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/app/client/src/components/editorComponents/CodeEditor/lintHelpers.ts b/app/client/src/components/editorComponents/CodeEditor/lintHelpers.ts index 65a7e34c27a..1d1cc01f56a 100644 --- a/app/client/src/components/editorComponents/CodeEditor/lintHelpers.ts +++ b/app/client/src/components/editorComponents/CodeEditor/lintHelpers.ts @@ -15,6 +15,7 @@ import { INVALID_JSOBJECT_START_STATEMENT, INVALID_JSOBJECT_START_STATEMENT_ERROR_CODE, } from "plugins/Linting/constants"; + export const getIndexOfRegex = ( str: string, regex: RegExp, @@ -137,6 +138,7 @@ export const getLintAnnotations = ( line, originalBinding, severity, + variableLength, variables, } = error; @@ -157,16 +159,18 @@ export const getLintAnnotations = ( }); } - let variableLength = 1; + let calculatedVariableLength = 1; // Find the variable with minimal length - if (variables) { + if (variableLength && variableLength > 0) { + calculatedVariableLength = variableLength; + } else if (variables) { for (const variable of variables) { if (variable) { - variableLength = - variableLength === 1 + calculatedVariableLength = + calculatedVariableLength === 1 ? String(variable).length - : Math.min(String(variable).length, variableLength); + : Math.min(String(variable).length, calculatedVariableLength); } } } @@ -205,7 +209,7 @@ export const getLintAnnotations = ( }; const to = { line: from.line, - ch: from.ch + variableLength, + ch: from.ch + calculatedVariableLength, }; annotations.push({ diff --git a/app/client/src/utils/DynamicBindingUtils.ts b/app/client/src/utils/DynamicBindingUtils.ts index 4e89443bcf1..68b25ccb305 100644 --- a/app/client/src/utils/DynamicBindingUtils.ts +++ b/app/client/src/utils/DynamicBindingUtils.ts @@ -436,6 +436,7 @@ export interface LintError extends DataTreeError { line: number; ch: number; originalPath?: string; + variableLength?: number; } export interface DataTreeEvaluationProps { From f213059695a5b25c9d3fd11b41b771c68ed35eed Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Fri, 20 Sep 2024 01:53:09 +0530 Subject: [PATCH 07/14] create: new pkg for linting --- app/client/package.json | 1 + app/client/yarn.lock | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/app/client/package.json b/app/client/package.json index 6a6e80842c8..0481ff2f2cf 100644 --- a/app/client/package.json +++ b/app/client/package.json @@ -125,6 +125,7 @@ "deep-diff": "^1.0.2", "downloadjs": "^1.4.7", "echarts": "^5.4.2", + "eslint-linter-browserify": "^9.10.0", "fast-deep-equal": "^3.1.3", "fast-sort": "^3.4.0", "fastdom": "^1.0.11", diff --git a/app/client/yarn.lock b/app/client/yarn.lock index 0c400dbe84f..074f5d17538 100644 --- a/app/client/yarn.lock +++ b/app/client/yarn.lock @@ -13346,6 +13346,7 @@ __metadata: eslint: ^8.51.0 eslint-config-prettier: ^9.0.0 eslint-import-resolver-babel-module: ^5.3.2 + eslint-linter-browserify: ^9.10.0 eslint-plugin-cypress: ^2.15.1 eslint-plugin-import: ^2.28.1 eslint-plugin-jest: ^27.4.2 @@ -18780,6 +18781,13 @@ __metadata: languageName: node linkType: hard +"eslint-linter-browserify@npm:^9.10.0": + version: 9.10.0 + resolution: "eslint-linter-browserify@npm:9.10.0" + checksum: fcaa6899a35e95b49c16a0c6ba6506a22fb89ec1e5f3cc61b6b1330f9291de243fb2fe0e52b893f8b344dbe515f8d67361e901b936ec1ae647b0725e30dcc783 + languageName: node + linkType: hard + "eslint-module-utils@npm:^2.8.0": version: 2.8.0 resolution: "eslint-module-utils@npm:2.8.0" From 0e47d57de2f82b567fc925346906e0ac05d35acc Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Fri, 20 Sep 2024 01:53:37 +0530 Subject: [PATCH 08/14] update: new opts based on linterversion --- app/client/src/plugins/Linting/constants.ts | 95 +++++++++++++++------ 1 file changed, 69 insertions(+), 26 deletions(-) diff --git a/app/client/src/plugins/Linting/constants.ts b/app/client/src/plugins/Linting/constants.ts index 6ea64ce2633..e36afdff4f7 100644 --- a/app/client/src/plugins/Linting/constants.ts +++ b/app/client/src/plugins/Linting/constants.ts @@ -2,32 +2,75 @@ import { ECMA_VERSION } from "@shared/ast"; import type { LintOptions } from "jshint"; import isEntityFunction from "./utils/isEntityFunction"; -export const lintOptions = (globalData: Record) => - ({ - indent: 2, - esversion: ECMA_VERSION, - eqeqeq: false, // Not necessary to use === - curly: false, // Blocks can be added without {}, eg if (x) return true - freeze: true, // Overriding inbuilt classes like Array is not allowed - undef: true, // Undefined variables should be reported as error - forin: false, // Doesn't require filtering for..in loops with obj.hasOwnProperty() - noempty: false, // Empty blocks are allowed - strict: false, // We won't force strict mode - unused: "strict", // Unused variables are not allowed - asi: true, // Tolerate Automatic Semicolon Insertion (no semicolons) - boss: true, // Tolerate assignments where comparisons would be expected - evil: false, // Use of eval not allowed - funcscope: true, // Tolerate variable definition inside control statements - sub: true, // Don't force dot notation - expr: true, // suppresses warnings about the use of expressions where normally you would expect to see assignments or function calls - // environments - browser: false, - worker: true, - mocha: false, - // global values - globals: globalData, - loopfunc: true, - }) as LintOptions; +export const lintOptions = ( + globalData: Record, + linterVersion = 1, +) => { + if (linterVersion === 1) { + return { + indent: 2, + esversion: ECMA_VERSION, + eqeqeq: false, // Not necessary to use === + curly: false, // Blocks can be added without {}, eg if (x) return true + freeze: true, // Overriding inbuilt classes like Array is not allowed + undef: true, // Undefined variables should be reported as error + forin: false, // Doesn't require filtering for..in loops with obj.hasOwnProperty() + noempty: false, // Empty blocks are allowed + strict: false, // We won't force strict mode + unused: "strict", // Unused variables are not allowed + asi: true, // Tolerate Automatic Semicolon Insertion (no semicolons) + boss: true, // Tolerate assignments where comparisons would be expected + evil: false, // Use of eval not allowed + funcscope: true, // Tolerate variable definition inside control statements + sub: true, // Don't force dot notation + expr: true, // suppresses warnings about the use of expressions where normally you would expect to see assignments or function calls + // environments + browser: false, + worker: true, + mocha: false, + // global values + globals: globalData, + loopfunc: true, + } as LintOptions; + } else { + const eslintGlobals: Record = {}; + + for (const key in globalData) { + //eslintGlobals[key] = globalData[key] ? "writable" : "readonly"; + eslintGlobals[key] = "readonly"; + } + + return { + languageOptions: { + ecmaVersion: ECMA_VERSION, + globals: eslintGlobals, + sourceType: "script", + }, + rules: { + indent: ["off", "tab"], + //indent: "off", + eqeqeq: "off", + curly: "off", + "no-extend-native": "error", + "no-undef": "error", + "guard-for-in": "off", + "no-empty": "off", + strict: "off", + "no-unused-vars": [ + "error", + { vars: "all", args: "all", ignoreRestSiblings: false }, + ], + //semi: ["error", "never"], + "no-cond-assign": "off", + "no-eval": "error", + "block-scoped-var": "off", + "dot-notation": "off", + "no-unused-expressions": "off", + "no-loop-func": "off", + }, + }; + } +}; export const JS_OBJECT_START_STATEMENT = "export default"; export const INVALID_JSOBJECT_START_STATEMENT = `JSObject must start with '${JS_OBJECT_START_STATEMENT}'`; export const INVALID_JSOBJECT_START_STATEMENT_ERROR_CODE = From 77cc9caa9032db4d0f227fd41b8630a23f0b6a62 Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Fri, 20 Sep 2024 01:54:43 +0530 Subject: [PATCH 09/14] create: conditional trigger for eslint --- app/client/src/plugins/Linting/types.ts | 1 + .../plugins/Linting/utils/getLintingErrors.ts | 186 +++++++++++++++--- 2 files changed, 163 insertions(+), 24 deletions(-) diff --git a/app/client/src/plugins/Linting/types.ts b/app/client/src/plugins/Linting/types.ts index a0de1b5a390..420a8e52a51 100644 --- a/app/client/src/plugins/Linting/types.ts +++ b/app/client/src/plugins/Linting/types.ts @@ -69,6 +69,7 @@ export interface getLintingErrorsProps { isJsObject: boolean; }; webworkerTelemetry: Record; + linterVersion?: number; } export interface getLintErrorsFromTreeProps { diff --git a/app/client/src/plugins/Linting/utils/getLintingErrors.ts b/app/client/src/plugins/Linting/utils/getLintingErrors.ts index eccbe7d0afc..c416942d81f 100644 --- a/app/client/src/plugins/Linting/utils/getLintingErrors.ts +++ b/app/client/src/plugins/Linting/utils/getLintingErrors.ts @@ -1,6 +1,7 @@ import type { Position } from "codemirror"; import type { LintError } from "utils/DynamicBindingUtils"; import { JSHINT as jshint } from "jshint"; +import { Linter } from "eslint-linter-browserify"; import type { LintError as JSHintError } from "jshint"; import { get, isEmpty, isNumber, keys } from "lodash"; import type { @@ -38,6 +39,7 @@ import { generate } from "astring"; import getInvalidModuleInputsError from "ee/plugins/Linting/utils/getInvalidModuleInputsError"; import { objectKeys } from "@appsmith/utils"; import { profileFn } from "UITelemetry/generateWebWorkerTraces"; +import { log, error } from "loglevel"; const EvaluationScriptPositions: Record = {}; @@ -130,6 +132,60 @@ function sanitizeJSHintErrors( }, []); } +function sanitizeESLintError( + lintErrors: Linter.LintMessage[], + scriptPos: Position, +): Linter.LintMessage[] { + return lintErrors.reduce((result: Linter.LintMessage[], lintError) => { + // Ignored errors should not be reported + if (IGNORED_LINT_ERRORS.includes(lintError.ruleId || "")) return result; + + /** Some error messages reference line numbers, + * Eg. Expected '{a}' to match '{b}' from line {c} and instead saw '{d}' + * these line numbers need to be re-calculated based on the binding location. + * Errors referencing line numbers outside the user's script should also be ignored + * */ + let message = lintError.message; + const matchedLines = message.match(/line \d/gi); + const lineNumbersInErrorMessage = new Set(); + let isInvalidErrorMessage = false; + + if (matchedLines) { + matchedLines.forEach((lineStatement) => { + const digitString = lineStatement.split(" ")[1]; + const digit = Number(digitString); + + if (isNumber(digit)) { + if (digit < scriptPos.line) { + // referenced line number is outside the scope of user's script + isInvalidErrorMessage = true; + } else { + lineNumbersInErrorMessage.add(digit); + } + } + }); + } + + if (isInvalidErrorMessage) return result; + + if (lineNumbersInErrorMessage.size) { + Array.from(lineNumbersInErrorMessage).forEach((lineNumber) => { + message = message.replaceAll( + `line ${lineNumber}`, + `line ${lineNumber - scriptPos.line + 1}`, + ); + }); + } + + result.push({ + ...lintError, + message, + }); + + return result; + }, []); +} + const getLintErrorMessage = ( reason: string, code: string, @@ -146,6 +202,47 @@ const getLintErrorMessage = ( } }; +function convertESHintErrorToAppsmithLintError( + eslintError: Linter.LintMessage, + script: string, + originalBinding: string, + scriptPos: Position, + //isJSObject = false, +): LintError { + const { column, endColumn = 0, line, message, ruleId } = eslintError; + + // Compute actual error position + const actualErrorLineNumber = line - scriptPos.line; + const actualErrorCh = + line === scriptPos.line + ? eslintError.column - scriptPos.ch + : eslintError.column; + //const lintErrorMessage = getLintErrorMessage( + // reason, + // code, + // [a, b, c, d], + // isJSObject, + //); + + return { + errorType: PropertyEvaluationErrorType.LINT, + raw: script, + severity: getLintSeverity(ruleId || "", message), + errorMessage: { + name: "LintingError", + message: message, + }, + errorSegment: "", + originalBinding, + // By keeping track of these variables we can highlight the exact text that caused the error. + variables: [], + variableLength: Math.max(endColumn - column, 0), + code: ruleId || "", + line: actualErrorLineNumber, + ch: actualErrorCh, + }; +} + function convertJsHintErrorToAppsmithLintError( jsHintError: JSHintError, script: string, @@ -188,6 +285,8 @@ function convertJsHintErrorToAppsmithLintError( export default function getLintingErrors({ data, + linterVersion = 2, // Use this version for elsint + //linterVersion = 1,// Use this version for jshint options, originalBinding, script, @@ -196,29 +295,68 @@ export default function getLintingErrors({ }: getLintingErrorsProps): LintError[] { const scriptPos = getEvaluationScriptPosition(scriptType); const lintingGlobalData = generateLintingGlobalData(data); - const lintingOptions = lintOptions(lintingGlobalData); - - profileFn( - "Linter", - // adding some metrics to compare the performance changes with eslint - { - linter: "JSHint", - linesOfCodeLinted: originalBinding.split("\n").length, - codeSizeInChars: originalBinding.length, - }, - webworkerTelemetry, - () => jshint(script, lintingOptions), - ); - const sanitizedJSHintErrors = sanitizeJSHintErrors(jshint.errors, scriptPos); - const jshintErrors: LintError[] = sanitizedJSHintErrors.map((lintError) => - convertJsHintErrorToAppsmithLintError( - lintError, - script, - originalBinding, - scriptPos, - options?.isJsObject, - ), - ); + const lintingOptions = lintOptions(lintingGlobalData, linterVersion); + let messages: Linter.LintMessage[] = []; + let linterErrors: LintError[] = []; + + try { + profileFn( + "Linter", + // adding some metrics to compare the performance changes with eslint + { + linter: linterVersion === 1 ? "JSHint" : "ESLint", + linesOfCodeLinted: originalBinding.split("\n").length, + codeSizeInChars: originalBinding.length, + }, + webworkerTelemetry, + () => { + if (linterVersion === 1) { + jshint(script, lintingOptions); + } else { + // Replace tabs with 2 spaces before linting + const tabSize = 2; + const codeForLinting = script.replace(/\t/g, " ".repeat(tabSize)); + + const linter = new Linter(); + + messages = linter.verify(codeForLinting, lintingOptions); + } + }, + ); + + if (linterVersion === 1) { + const sanitizedJSHintErrors = sanitizeJSHintErrors( + jshint.errors, + scriptPos, + ); + + linterErrors = sanitizedJSHintErrors.map((lintError) => + convertJsHintErrorToAppsmithLintError( + lintError, + script, + originalBinding, + scriptPos, + options?.isJsObject, + ), + ); + } else { + const sanitizedESLintError = sanitizeESLintError(messages, scriptPos); + + linterErrors = sanitizedESLintError.map((lintError) => + convertESHintErrorToAppsmithLintError( + lintError, + script, + originalBinding, + scriptPos, + //options?.isJsObject, + ), + ); + log("ayush", messages, linterErrors); + //console.log(sanitizeESLintError(messages, scriptPos)); + } + } catch (e) { + error("ayush errors", e); + } const customLintErrors = getCustomErrorsFromScript( script, data, @@ -227,7 +365,7 @@ export default function getLintingErrors({ options?.isJsObject, ); - return jshintErrors.concat(customLintErrors); + return linterErrors.concat(customLintErrors); } function getInvalidWidgetPropertySetterErrors({ From 085a5436e36a9d95e73bd0a89b6146e6e934e3fd Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Tue, 24 Sep 2024 22:25:51 +0530 Subject: [PATCH 10/14] update: added new feature flag --- app/client/src/ce/entities/FeatureFlag.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/client/src/ce/entities/FeatureFlag.ts b/app/client/src/ce/entities/FeatureFlag.ts index 69ac4b7e9c6..3e3d5cf4912 100644 --- a/app/client/src/ce/entities/FeatureFlag.ts +++ b/app/client/src/ce/entities/FeatureFlag.ts @@ -39,6 +39,7 @@ export const FEATURE_FLAG = { rollout_js_enabled_one_click_binding_enabled: "rollout_js_enabled_one_click_binding_enabled", rollout_side_by_side_enabled: "rollout_side_by_side_enabled", + rollout_eslint_enabled: "rollout_eslint_enabled", ab_learnability_ease_of_initial_use_enabled: "ab_learnability_ease_of_initial_use_enabled", ab_learnability_discoverability_collapse_all_except_data_enabled: @@ -78,6 +79,7 @@ export const DEFAULT_FEATURE_FLAG_VALUE: FeatureFlags = { release_actions_redesign_enabled: false, rollout_remove_feature_walkthrough_enabled: true, rollout_js_enabled_one_click_binding_enabled: true, + rollout_eslint_enabled: false, rollout_side_by_side_enabled: false, ab_learnability_ease_of_initial_use_enabled: true, ab_learnability_discoverability_collapse_all_except_data_enabled: true, From 7c5f36e38aa8832be57e1ff2fdad6971864a66f9 Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Tue, 24 Sep 2024 22:26:26 +0530 Subject: [PATCH 11/14] update: enums to dictate linter version --- app/client/src/plugins/Linting/constants.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/client/src/plugins/Linting/constants.ts b/app/client/src/plugins/Linting/constants.ts index e36afdff4f7..2e72dba5335 100644 --- a/app/client/src/plugins/Linting/constants.ts +++ b/app/client/src/plugins/Linting/constants.ts @@ -2,11 +2,16 @@ import { ECMA_VERSION } from "@shared/ast"; import type { LintOptions } from "jshint"; import isEntityFunction from "./utils/isEntityFunction"; +export const enum LINTER_VERSION { + "JSHINT" = "JSHint", + "ESLINT" = "ESLint", +} + export const lintOptions = ( globalData: Record, - linterVersion = 1, + linterVersion = LINTER_VERSION.JSHINT, ) => { - if (linterVersion === 1) { + if (linterVersion === LINTER_VERSION.JSHINT) { return { indent: 2, esversion: ECMA_VERSION, From 7437f88d042f7a8c05a9c2041a6c1caa40f032fb Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Tue, 24 Sep 2024 22:26:56 +0530 Subject: [PATCH 12/14] update: started using feature flag to control linter engine --- .../plugins/Linting/utils/getLintingErrors.ts | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/app/client/src/plugins/Linting/utils/getLintingErrors.ts b/app/client/src/plugins/Linting/utils/getLintingErrors.ts index c416942d81f..d02d184e1df 100644 --- a/app/client/src/plugins/Linting/utils/getLintingErrors.ts +++ b/app/client/src/plugins/Linting/utils/getLintingErrors.ts @@ -26,6 +26,7 @@ import { IGNORED_LINT_ERRORS, lintOptions, SUPPORTED_WEB_APIS, + LINTER_VERSION, } from "../constants"; import type { getLintingErrorsProps } from "../types"; import { JSLibraries } from "workers/common/JSLibrary"; @@ -40,9 +41,24 @@ import getInvalidModuleInputsError from "ee/plugins/Linting/utils/getInvalidModu import { objectKeys } from "@appsmith/utils"; import { profileFn } from "UITelemetry/generateWebWorkerTraces"; import { log, error } from "loglevel"; +import { WorkerEnv } from "workers/Evaluation/handlers/workerEnv"; +import { FEATURE_FLAG } from "ee/entities/FeatureFlag"; const EvaluationScriptPositions: Record = {}; +function getLinterVersion() { + let linterVersion = LINTER_VERSION.JSHINT; + + const flagValues = WorkerEnv.getFeatureFlags(); + const flagName = FEATURE_FLAG.rollout_eslint_enabled; + + if (flagName in flagValues && flagValues[flagName]) { + linterVersion = LINTER_VERSION.ESLINT; + } + + return linterVersion; +} + function getEvaluationScriptPosition(scriptType: EvaluationScriptType) { if (isEmpty(EvaluationScriptPositions)) { // We are computing position of <