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

Fix remove console.* statements #421

Merged
merged 6 commits into from Feb 20, 2017
Merged

Fix remove console.* statements #421

merged 6 commits into from Feb 20, 2017

Conversation

boopathi
Copy link
Member

@boopathi boopathi added the Tag: Bug Fix Pull Request fixes a bug label Feb 12, 2017
@boopathi boopathi requested a review from kangax February 12, 2017 21:50
@boopathi
Copy link
Member Author

/cc @DrewML

@vigneshshanmugam
Copy link
Member

👍

@mikesherov
Copy link

Hi! I'm a first time contributor to babili, and had a few questions/comments about this pull request!

  1. Considering babili is es6 aware, why the change from arrowFunctionExpressions to normal ones?
  2. I think this PR misses console.log.call and console.log.apply.

Thanks!

@hzoo
Copy link
Member

hzoo commented Feb 18, 2017

Wow @mikesherov's here 😄

},
};

function isConsole(memberExpr) {

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.

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()

@boopathi
Copy link
Member Author

@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.

@mikesherov
Copy link

Got it.

@boopathi
Copy link
Member Author

Added call and apply.

Copy link

@mikesherov mikesherov left a 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()) {

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()) {

Copy link
Member Author

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());
}
Copy link
Contributor

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?

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tag: Bug Fix Pull Request fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants