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.

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

Merged
merged 20 commits into from Jul 11, 2021
Merged

GSoC'21: adding to FES phase 1 #5305

merged 20 commits into from Jul 11, 2021

Conversation

Aloneduckling
Copy link
Contributor

@Aloneduckling Aloneduckling commented Jun 11, 2021

Resolves #5304

Changes:

Changes made to browser_errors.js, fes_core.js, and translation.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

@Aloneduckling
Copy link
Contributor Author

This is a draft pull request. I will get feedback from my mentors and update the code.

@lm-n lm-n self-requested a review June 16, 2021 12:35
@Aloneduckling
Copy link
Contributor Author

Aloneduckling commented Jun 19, 2021

The checks fail because of a recent change in the report() function that FES uses to log friendly error messages.
Link to the PR: #5317

Copy link
Member

@lm-n lm-n left a 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 Show resolved Hide resolved
translations/en/translation.json Outdated Show resolved Hide resolved
src/core/friendly_errors/browser_errors.js Show resolved Hide resolved
"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}}"
Copy link
Member

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

Copy link
Contributor Author

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}}

Copy link
Member

Choose a reason for hiding this comment

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

sounds good to me!

@lm-n
Copy link
Member

lm-n commented Jun 26, 2021

@Aloneduckling for fes_core.js and error_helpers.js please add in-line comments to document each case/test.

@lm-n lm-n requested a review from ghalestrilo June 30, 2021 22:08
translations/en/translation.json Outdated Show resolved Hide resolved
translations/en/translation.json Outdated Show resolved Hide resolved
"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}}"
Copy link
Member

Choose a reason for hiding this comment

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

sounds good to me!

Copy link
Member

@lm-n lm-n left a 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.

@Aloneduckling
Copy link
Contributor Author

@lm-n, I have changes the styles and updated the error messages please have a look

Copy link
Contributor

@ghalestrilo ghalestrilo left a 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.

translations/en/translation.json Outdated Show resolved Hide resolved
"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}}"
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

test/unit/core/error_helpers.js Outdated Show resolved Hide resolved
test/unit/core/error_helpers.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ghalestrilo ghalestrilo left a 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

@lm-n lm-n marked this pull request as ready for review July 11, 2021 14:00
Copy link
Member

@lm-n lm-n left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GSoC'21: More error messages for FES's fesErrorMonitor
4 participants