-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
fix: unsupported function defined inside a condition block. #7263 #7411
Conversation
For maintainers only:
|
A separate PR for fixing remaining problems of #7210. |
|
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.
LGTM. However, please fix styling issues.
@ooflorent hoisted function does not work inside condition block under FireFox. |
@ooflorent It seems like my fault, and the result should not be an issue, as it has been executed within another event loop: |
So we can close it? |
It depends on you all. In my opinion, it is still not proper to declare function inside a condition block, a.k.a inner-declaration, according to a document of ESLint, and I will submit another commit for this, and revert the commit of #7210. |
Hoisted variables should be avoided, like using |
@sokra It seems that the "stats test cases" has not already been matched? |
Use a varaible declaration outside a condition block instead, according to https://eslint.org/docs/rules/no-inner-declarations
ff1a929
to
9c3d7a2
Compare
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
]), | ||
`}, ${chunkLoadTimeout});`, | ||
"function onScriptComplete(event) {", | ||
"onScriptComplete = function (event) {", |
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.
function onScriptComplete(event) {
was fine
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.
As an inner declaration, it was not fine to some extends.
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.
Hi @aleen42. Just a little hint from a friendly bot about the best practice when submitting pull requests:
You don't have to change it for this PR, just make sure to follow this hint the next time you submit a PR. |
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.
Ok let's use that
@aleen42 please also sign the CLA |
I have signed it and can u check this? @sokra |
Thanks |
Issue Reference
fixes #7263
What kind of change does this PR introduce?
Call the method
onScriptComplete
after its declaration as there should ben an error of references under Friefox 40, when it has been declared inside a conditon block, rather than the top block inside a function.Did you add tests for your changes?
No needs.
Does this PR introduce a breaking change?
Solve the problem under Firefox 40.
What needs to be documented once your changes are merged?
No needs.