Protei/PromucFlow_constructor
2
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

feat: address few bug with linting and improve highlighting (#7287)

Browse Source 
* show lint errors as warnings

* add initial code

* adjust editor positions

* update pr comments

* mark jshint errors

* reset changes

* remove unused prop

* fix test errors

* remove unused imports

* dont show warning in the error counter

* show yellow if warning

* remove active error functionality

* update linter to use async functionality

* update linter messages

* update binding positions

* fix evaluate tests

* dont show undefined errors in debugger

* move lint code to separate file

* update testes

* remove unused import

* update tests

* address pr comments

* add comment to explain why

* replace proper regex

* fix undefined message error

* Update styling for warnings

* address position issue in the linter

* Fix failing tests

* Merge 'release' on to  'Feature/linting-errors'

* add console as global object

* address lint issues

* fix requested linting erros for release

* fix breaking issue

* remove unwanted code

* revert unrelated changes

* remove unnecessary file

* add extra libraries to jshint data

* import lodash functions that are used

* update jshint settings

* Fix failing test

* don't show lint errors if there is a parsing issue

* update jshint to latest version
This commit is contained in:
Vinod 2021-09-17 16:01:45 +05:30 committed by GitHub
parent 6baa5c8489
commit 2bd324a5aa
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 159 additions and 150 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/client/package.json
View File

@ -81,7 +81,7 @@
"interweave-autolink": "^4.4.2",
"js-beautify": "^1.14.0",
"js-sha256": "^0.9.0",
"jshint": "^2.12.0",
"jshint": "^2.13.1",
"json-fn": "^1.1.1",
"lint-staged": "^9.2.5",
"localforage": "^1.7.3",

15
app/client/src/components/editorComponents/CodeEditor/index.tsx
View File

@ -268,6 +268,9 @@ class CodeEditor extends Component<Props, State> {
this.props.showLightningMenu,
this.props.additionalDynamicData,
);
if (getFeatureFlags().LINTING) {
this.lintCode(editor);
}
};
// Finally create the Codemirror editor
@ -463,10 +466,10 @@ class CodeEditor extends Component<Props, State> {
}
};
lintCode() {
lintCode(editor: CodeMirror.Editor) {
const { dataTreePath, dynamicData } = this.props;
if (!dataTreePath || !this.updateLintingCallback) {
if (!dataTreePath || !this.updateLintingCallback || !editor) {
return;
}
@ -478,11 +481,9 @@ class CodeEditor extends Component<Props, State> {
let annotations: Annotation[] = [];
if (this.editor) {
annotations = getLintAnnotations(this.editor.getValue(), errors);
}
annotations = getLintAnnotations(editor.getValue(), errors);
this.updateLintingCallback(this.editor, annotations);
this.updateLintingCallback(editor, annotations);
}
static updateMarkings = (
@ -586,7 +587,7 @@ class CodeEditor extends Component<Props, State> {
/* Evaluation results for snippet snippets */
if (getFeatureFlags().LINTING) {
this.lintCode();
this.lintCode(this.editor);
}
const showEvaluatedValue =

81
app/client/src/components/editorComponents/CodeEditor/lintHelpers.test.ts
View File

@ -39,70 +39,49 @@ describe("getKeyPositionsInString()", () => {
});
describe("getLintAnnotations()", () => {
const { LINT, PARSE } = PropertyEvaluationErrorType;
const { ERROR, WARNING } = Severity;
const { LINT } = PropertyEvaluationErrorType;
const { WARNING } = Severity;
it("should return proper annotations", () => {
const value = `Hello {{ world == test }}\n {{text}}`;
const value = `Hello {{ world == test }}`;
const errors: EvaluationError[] = [
{
errorMessage: "Expected '===' and instead saw '=='.",
severity: WARNING,
raw:
"\n function closedFunction () {\n const result = world == test \n return result;\n }\n closedFunction()\n ",
errorType: LINT,
originalBinding: "world == test ",
errorSegment: " const result = world == test ",
raw:
"\n function closedFunction () {\n const result = world == test \n return result;\n }\n closedFunction()\n ",
severity: WARNING,
errorMessage: "Expected '===' and instead saw '=='.",
errorSegment: " const result = world == test ",
originalBinding: " world == test ",
variables: ["===", "==", null, null],
code: "W116",
line: 0,
ch: 8,
},
{
errorType: LINT,
raw:
"\n function closedFunction () {\n const result = world == test \n return result;\n }\n closedFunction()\n ",
"\n function closedFunction () {\n const result = world == test \n return result;\n }\n closedFunction()\n ",
severity: WARNING,
errorMessage: "'world' is not defined.",
errorSegment: " const result = world == test ",
originalBinding: "world == test ",
errorSegment: " const result = world == test ",
originalBinding: " world == test ",
variables: ["world", null, null, null],
code: "W117",
line: 0,
ch: 2,
},
{
errorType: LINT,
raw:
"\n function closedFunction () {\n const result = world == test \n return result;\n }\n closedFunction()\n ",
severity: WARNING,
errorMessage: "'test' is not defined.",
errorSegment: " const result = world == test ",
originalBinding: "world == test ",
severity: WARNING,
raw:
"\n function closedFunction () {\n const result = world == test \n return result;\n }\n closedFunction()\n ",
errorType: LINT,
originalBinding: " world == test ",
errorSegment: " const result = world == test ",
variables: ["test", null, null, null],
code: "W117",
},
{
errorType: PARSE,
raw:
"\n function closedFunction () {\n const result = world == test \n return result;\n }\n closedFunction()\n ",
severity: ERROR,
errorMessage: "ReferenceError: world is not defined",
originalBinding: " world == test ",
},
{
errorMessage: "'text' is not defined.",
severity: WARNING,
raw:
"\n function closedFunction () {\n const result = text\n return result;\n }\n closedFunction()\n ",
errorType: LINT,
originalBinding: "text",
errorSegment: " const result = text",
variables: ["text", null, null, null],
code: "W117",
},
{
errorMessage: "ReferenceError: text is not defined",
severity: WARNING,
raw:
"\n function closedFunction () {\n const result = text\n return result;\n }\n closedFunction()\n ",
errorType: PARSE,
originalBinding: "text",
line: 0,
ch: 11,
},
];
@ -144,18 +123,6 @@ describe("getLintAnnotations()", () => {
message: "'test' is not defined.",
severity: "warning",
},
{
from: {
line: 1,
ch: 4,
},
to: {
line: 1,
ch: 8,
},
message: "'text' is not defined.",
severity: "warning",
},
]);
});
});

97
app/client/src/components/editorComponents/CodeEditor/lintHelpers.ts
View File

@ -1,4 +1,4 @@
import _ from "lodash";
import { last, isNumber } from "lodash";
import { Annotation, Position } from "codemirror";
import {
EvaluationError,
@ -44,7 +44,7 @@ export const getKeyPositionInString = (
for (const index of indices) {
const substr = str.substr(0, index);
const substrLines = substr.split("\n");
const ch = _.last(substrLines)?.length || 0;
const ch = last(substrLines)?.length || 0;
const line = substrLines.length - 1;
positions.push({ line, ch });
@ -63,68 +63,65 @@ export const getLintAnnotations = (
const lintErrors = errors.filter(
(error) => error.errorType === PropertyEvaluationErrorType.LINT,
);
const lines = value.split("\n");
lintErrors.forEach((error) => {
const { errorMessage, originalBinding, severity, variables } = error;
const {
ch,
errorMessage,
line,
originalBinding,
severity,
variables,
} = error;
if (!originalBinding) {
return annotations;
}
// We find the location of binding in the editor value and then
// we find locations of jshint variables (a, b, c, d) in the binding and highlight them
let variableLength = 1;
// Find the variable with minimal length
if (variables) {
for (const variable of variables) {
if (variable) {
variableLength =
variableLength === 1
? variable.length
: Math.min(variable.length, variableLength);
}
}
}
const bindingPositions = getKeyPositionInString(value, originalBinding);
for (const bindingLocation of bindingPositions) {
if (variables?.filter((v) => v).length) {
for (let variable of variables) {
if (typeof variable === "number") {
variable = variable.toString();
}
if (variable && originalBinding.includes(variable)) {
const variableLocations = getKeyPositionInString(
originalBinding,
variable,
);
if (isNumber(line) && isNumber(ch)) {
for (const bindingLocation of bindingPositions) {
const currentLine = bindingLocation.line + line;
const lineContent = lines[currentLine] || "";
const currentCh = originalBinding.includes("\n")
? ch
: bindingLocation.ch + ch;
// Jshint counts \t as two characters and codemirror counts it as 1.
// So we need to subtract number of tabs to get accurate position
const tabs = lineContent.substr(0, currentCh).match(/\t/g)?.length || 0;
for (const variableLocation of variableLocations) {
const from = {
line: bindingLocation.line + variableLocation.line,
// if the binding is a multiline function we need to
// use jshint variable position as the starting point
ch:
variableLocation.line > 0
? variableLocation.ch
: variableLocation.ch + bindingLocation.ch,
};
const to = {
line: from.line,
ch: from.ch + variable.length,
};
const annotation = {
from,
to,
message: errorMessage,
severity,
};
annotations.push(annotation);
}
}
}
} else {
const from = bindingLocation;
const to = { line: from.line, ch: from.ch + 3 };
const annotation = {
const from = {
line: currentLine,
ch: currentCh - tabs - 1,
};
const to = {
line: from.line,
ch: from.ch + variableLength,
};
annotations.push({
from,
to,
message: errorMessage,
severity,
};
annotations.push(annotation);
});
}
} else {
// Don't show linting errors if code has parsing errors
return [];
}
});
return annotations;

4
app/client/src/sagas/PostEvaluationSagas.ts
View File

@ -130,10 +130,12 @@ function logLatestEvalPropertyErrors(
// Reformatting eval errors here to a format usable by the debugger
const errorMessages = errors.map((e) => {
// Error format required for the debugger
return {
const formattedError = {
message: e.errorMessage,
type: e.errorType,
};
return formattedError;
});
const analyticsData = isWidget(entity)

4
app/client/src/utils/DynamicBindingUtils.ts
View File

@ -353,8 +353,10 @@ export type EvaluationError = {
severity: Severity.WARNING | Severity.ERROR;
errorSegment?: string;
originalBinding?: string;
variables?: (string | undefined | null | number)[];
variables?: (string | undefined | null)[];
code?: string;
line?: number;
ch?: number;
};
export interface DataTreeEvaluationProps {

4
app/client/src/workers/evaluate.test.ts
View File

@ -47,10 +47,12 @@ describe("evaluate", () => {
triggers: [],
errors: [
{
ch: 1,
code: "W117",
errorMessage: "'wrongJS' is not defined.",
errorSegment: " const result = wrongJS",
errorType: "LINT",
line: 0,
raw: `
function closedFunction () {
const result = wrongJS
@ -58,7 +60,7 @@ describe("evaluate", () => {
}
closedFunction()
`,
severity: "warning",
severity: "error",
originalBinding: "wrongJS",
variables: ["wrongJS", undefined, undefined, undefined],
},

95
app/client/src/workers/evaluate.ts
View File

@ -8,9 +8,10 @@ import {
import unescapeJS from "unescape-js";
import { JSHINT as jshint } from "jshint";
import { Severity } from "entities/AppsmithConsole";
import { Position } from "codemirror";
import { AppsmithPromise, enhanceDataTreeWithFunctions } from "./Actions";
import { ActionDescription } from "entities/DataTree/actionTriggers";
import { isEmpty } from "lodash";
import { isEmpty, last } from "lodash";
export type EvalResult = {
result: any;
@ -24,34 +25,44 @@ export enum EvaluationScriptType {
TRIGGERS = "TRIGGERS",
}
const evaluationScripts: Record<
EvaluationScriptType,
(script: string) => string
> = {
[EvaluationScriptType.EXPRESSION]: (script: string) => `
const evaluationScriptsPos: Record<EvaluationScriptType, string> = {
[EvaluationScriptType.EXPRESSION]: `
function closedFunction () {
const result = ${script}
const result = <<script>>
return result;
}
closedFunction()
`,
[EvaluationScriptType.ANONYMOUS_FUNCTION]: (script) => `
[EvaluationScriptType.ANONYMOUS_FUNCTION]: `
function callback (script) {
const userFunction = script;
const result = userFunction.apply(self, ARGUMENTS);
return result;
}
callback(${script})
callback(<<script>>)
`,
[EvaluationScriptType.TRIGGERS]: (script) => `
[EvaluationScriptType.TRIGGERS]: `
function closedFunction () {
const result = ${script};
return result;
const result = <<script>>
return result
}
closedFunction();
`,
};
const getPositionInEvaluationScript = (
type: EvaluationScriptType,
): Position => {
const script = evaluationScriptsPos[type];
const index = script.indexOf("<<script>>");
const substr = script.substr(0, index);
const lines = substr.split("\n");
const lastLine = last(lines) || "";
return { line: lines.length, ch: lastLine.length };
};
const getScriptType = (
evalArguments?: Array<any>,
isTriggerBased = false,
@ -67,47 +78,64 @@ const getScriptType = (
const getScriptToEval = (
userScript: string,
evalArguments?: Array<any>,
isTriggerBased = false,
type: EvaluationScriptType,
): string => {
const scriptType = getScriptType(evalArguments, isTriggerBased);
return evaluationScripts[scriptType](userScript);
return evaluationScriptsPos[type].replace("<<script>>", userScript);
};
const getLintingErrors = (
script: string,
data: Record<string, unknown>,
originalBinding: string,
scriptPos: Position,
): EvaluationError[] => {
const globalData: Record<string, boolean> = {};
Object.keys(data).forEach((datum) => (globalData[datum] = false));
Object.keys(data).forEach((datum) => (globalData[datum] = true));
// Jshint shouldn't throw errors for additional libraries
extraLibraries.forEach((lib) => (globalData[lib.accessor] = true));
globalData.console = true;
const options = {
indent: 2,
esversion: 7,
eqeqeq: true,
curly: true,
freeze: true,
undef: true,
unused: true,
asi: true,
esversion: 8, // For async/await support
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: false, // Unused variables are 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
// environments
browser: true,
worker: true,
mocha: false,
// global values
globals: globalData,
};
jshint(script, options);
return jshint.errors.map((lintError) => {
const ch = lintError.character;
return {
errorType: PropertyEvaluationErrorType.LINT,
raw: script,
severity: lintError.code.startsWith("W")
? Severity.WARNING
: Severity.ERROR,
// We are forcing warnings to errors and removing unwanted JSHint checks
severity: Severity.ERROR,
errorMessage: lintError.reason,
errorSegment: lintError.evidence,
originalBinding,
// By keeping track of these variables we can highlight the exact text that caused the error.
variables: [lintError.a, lintError.b, lintError.c, lintError.d],
code: lintError.code,
line: lintError.line - scriptPos.line,
ch: lintError.line === scriptPos.line ? ch - scriptPos.ch : ch,
};
});
};
@ -125,7 +153,11 @@ export default function evaluate(
// makes the final function invalid. We also unescape any escaped characters
// so that eval can happen
const unescapedJS = unescapeJS(js.replace(beginsWithLineBreakRegex, ""));
const script = getScriptToEval(unescapedJS, evalArguments, isTriggerBased);
const scriptType = getScriptType(evalArguments, isTriggerBased);
const script = getScriptToEval(unescapedJS, scriptType);
// We are linting original js binding,
// This will make sure that the characted count is not messed up when we do unescapejs
const scriptToLint = getScriptToEval(js, scriptType);
return (function() {
let errors: EvaluationError[] = [];
let result;
@ -167,7 +199,12 @@ export default function evaluate(
});
});
}
errors = getLintingErrors(script, GLOBAL_DATA, unescapedJS);
errors = getLintingErrors(
scriptToLint,
GLOBAL_DATA,
js,
getPositionInEvaluationScript(scriptType),
);
///// Adding extra libraries separately
extraLibraries.forEach((library) => {

7
app/client/yarn.lock
View File

@ -11388,9 +11388,10 @@ jsesc@~0.5.0:
version "0.5.0"
resolved "https://registry.npmjs.org/jsesc/-/jsesc-0.5.0.tgz"
jshint@^2.12.0:
version "2.13.0"
resolved "https://registry.npmjs.org/jshint/-/jshint-2.13.0.tgz"
jshint@^2.13.1:
version "2.13.1"
resolved "https://registry.yarnpkg.com/jshint/-/jshint-2.13.1.tgz#16bbbecdbb4564d3758d9de4f24926f8c7f8f835"
integrity sha512-vymzfR3OysF5P774x6zYv0bD4EpH6NWRxpq54wO9mA9RuY49yb1teKSICkLx2Ryx+mfzlVVNNbTBtsRtg78t7g==
dependencies:
cli "~1.0.0"
console-browserify "1.1.x"