-
-
Notifications
You must be signed in to change notification settings - Fork 225
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 remove console.* statements #421
Conversation
boopathi
commented
Feb 12, 2017
- (Fix SyntaxError after transform-remove-console #374)
- (Fix Account for removal of consequent inside ternary for no-console plugin. Fixes #374 #375)
/cc @DrewML |
👍 |
Hi! I'm a first time contributor to babili, and had a few questions/comments about this pull request!
Thanks! |
Wow @mikesherov's here 😄 |
}, | ||
}; | ||
|
||
function isConsole(memberExpr) { |
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.
Seems this needs to check for console.log.call and console.log.apply.
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.
And the test needs to check both a = console.log.call
and console.log.call()
@mikesherov this will change it(not transforming function to arrow. But use arrow directly by checking if the target the user intended supports arrow) - #428. |
Got it. |
Added |
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.
Thanks for addressing!
MemberExpression: { | ||
exit(path) { | ||
if (!isConsole(path)) return; | ||
if (!path.parentPath.isMemberExpression()) { |
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.
Is this clearer as:
if (isConsole(path) && !path.parentPath.isMemberExpression()) {
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.
Yup!. Thanks.
exit(path) { | ||
if (isConsole(path) && !path.parentPath.isMemberExpression()) { | ||
path.replaceWith(createNoop()); | ||
} |
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.
It looks not work with console.err = xx
(React Native have similar usage), maybe we can just parentPath.remove()
if path key is left
and parentPath is assignment expression?
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.
Interesting, So, I would assume it to be replaced like this -
// in
console.foo = foo;
// out
console.foo = function () {};
which looks like the right thing to do. because it is hard to detect all usages of console.foo
in code
const x = "foo";
const y = console;
y[x]();
and will lead to runtime error.