-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
GSoC'21: adding to FES phase 1 #5305
Conversation
This is a draft pull request. I will get feedback from my mentors and update the code. |
The checks fail because of a recent change in the report() function that FES uses to log friendly error messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! I've added some comments! Good job @Aloneduckling
translations/en/translation.json
Outdated
"notfuncObj": "There's an error as \"{{symbol}}\" could not be called as a function {{location}}.\nVerify whether \"{{obj}}\" has \"{{symbol}}\" in it and check the spelling, letter-casing (JavaScript is case-sensitive) and its type.\nFor more: {{url}}" | ||
"notfuncObj": "There's an error as \"{{symbol}}\" could not be called as a function {{location}}.\nVerify whether \"{{obj}}\" has \"{{symbol}}\" in it and check the spelling, letter-casing (JavaScript is case-sensitive) and its type.\nFor more: {{url}}", | ||
"readFromNull": "\nError ▶️ The property of null can't be read. In javascript the value null indicates that an object has no value.\nFor more: \n{{url1}}\n{{url2}}", | ||
"readFromUndefined": "\nError ▶️ The property of something undefined can't be read. Check the line number in error below and make sure the variable which is being operated is not undefined.\nFor more: \n{{url1}}\n{{url2}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this message could be "\nError ▶️ \"{{symbol}}\" is undefined, the property of undefined cannot be read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lm-n we don't have access to {{symbol}} in this error because the browser itself doesn't provide it.
What if we change it to:
\nError ▶️ cannot read property of undefined. Check the line number in the error and make sure the variable which is being operated is not undefined.\nFor more: \n{{url1}}\n{{url2}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me!
@Aloneduckling for |
translations/en/translation.json
Outdated
"notfuncObj": "There's an error as \"{{symbol}}\" could not be called as a function {{location}}.\nVerify whether \"{{obj}}\" has \"{{symbol}}\" in it and check the spelling, letter-casing (JavaScript is case-sensitive) and its type.\nFor more: {{url}}" | ||
"notfuncObj": "There's an error as \"{{symbol}}\" could not be called as a function {{location}}.\nVerify whether \"{{obj}}\" has \"{{symbol}}\" in it and check the spelling, letter-casing (JavaScript is case-sensitive) and its type.\nFor more: {{url}}", | ||
"readFromNull": "\nError ▶️ The property of null can't be read. In javascript the value null indicates that an object has no value.\nFor more: \n{{url1}}\n{{url2}}", | ||
"readFromUndefined": "\nError ▶️ The property of something undefined can't be read. Check the line number in error below and make sure the variable which is being operated is not undefined.\nFor more: \n{{url1}}\n{{url2}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress! :) Just commented on a couple of small things mostly about style.
@lm-n, I have changes the styles and updated the error messages please have a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good! I made a few comments on error messages and test code structure, but it's really up to you which of these changes to make. Let me know how you feel about them.
"invalidToken": "\nSyntax Error ▶️ Found a symbol that JavaScript doesn't recognize or didn't expect at it's place.\nFor more: {{url}}", | ||
"missingInitializer": "\nSyntax Error ▶️ A const variable is declared but not initialized. In JavaScript, an initializer for a const is required. A value must be specified in the same statement in which the variable is declared. Check the line number in the error and assign the const variable a value.\nFor more: {{url}}", | ||
"redeclaredVariable": "\nSyntax Error ▶️ \"{{symbol}}\" is being redeclared. JavaScript doesn't allow declaring a variable more than once. Check the line number in error for redeclaration of the variable.\nFor more: {{url}}", | ||
"unexpectedToken": "\nSyntax Error ▶️ Symbol present at a place that wasn't expected.\nUsually this is due to a typo. Check the line number in the error for anything missing/extra.\nFor more: {{url}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"unexpectedToken" often happens when the previous line is malformed. We could suggest the user to check the previous line as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghalestrilo that is a great idea. However, In many cases, the error is in the same line as indicated in the error message so it might create confusion for the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I think this message is good. I am happy to defer though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, let's leave it as is for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great improvement @Aloneduckling! I'll go ahead and merge!
Resolves #5304
Changes:
Changes made to
browser_errors.js
,fes_core.js
, andtranslation.json
FES would now handle these new browser errors :
Reference Errors:
cannotAccess
Syntax Errors:
redeclaredVariable
missingInitilizer
badReturnOrYield
Type Errors:
readFromNull
readFromUndefined
constAssign
Screenshots of the change:
PR Checklist
npm run lint
passes