Fix remove hooks for Assignment/Conditional

[danez]

Q	A
Bug fix?	yes
Breaking change?	no
New feature?	no
Deprecations?	no
Spec compliancy?	no
Tests added/pass?	yes
Fixed tickets	Fixes babel/minify#300 facebook/react-native#10412
License	MIT
Doc PR	no
Dependency Changes	no

This ensures that if any subnode of ConditionalExpressions and right of AssignmentExpression are removed they get replaced by undefined (or void 0).

I'm not sure what should happen if the left-hand-side of an assignment gets removed.

x = someCall();

If now someone removes the left-hand-side we probably need to turn it into

someCall();

But I'm not sure. In certain cases, we can probably also remove the whole thing alltogether if there can be no side-effects:

x = a;

//cc @boopathi @kangax

[danez]

In that case we could also replace void 0 with a simple blockStatement I think?

x = () => {};

Which would also be shorter.

[boopathi]

yes 👍

[codecov-io]

Current coverage is 89.38% (diff: 100%)

Merging #4896 into master will increase coverage by 0.03%

@@             master      #4896   diff @@
==========================================
  Files           196        196          
  Lines         14022      14022          
  Methods        1460       1460          
  Messages          0          0          
  Branches       3263       3263          
==========================================
+ Hits          12529      12534     +5   
+ Misses         1493       1488     -5   
  Partials          0          0          

Powered by Codecov. Last update 1d9e509...c3960ca

[kangax]

I see few options here. When we remove a node and that makes for an invalid AST, we could either:

• Throw an error / disallow it
• Try to replace it with something that still preserves correct AST
• Try to remove parent nodes until the AST is valid

Since we're probably going with the 2nd one, the further options are to either:

• Try to infer the context and replace it with something sensible (e.g. undefined in place of an Expression and empty statement in case of a Statement)
• Give user an option to specify what to replace it with

The API such as path.remove(optionalReplacement) is an interesting option, although might be a little confusing. We also already have path.replaceWith. But it's not too terrible of an idea.

The issues with replacing expression with undefined is that it could lead to subtle behavior changes in code, such as the case with x = <deleted node>. In that case, 3rd option — where we delete parent nodes — would allow us to traverse up and delete the entire AssignmentExpression.

Another example I can think of is removing an argument from the catch clause, which isn't allowed. We can't replace it with undefined, so we need to either leave it (disallow removal) or remove entire try-catch statement.

Thoughts?

[boopathi]

Throwing in places where a required Field went missing in parentNode is a better idea - tells the plugin author that you're not allowed to remove this - you can only replace it with something else, or remove the entire parent.

I think babel shouldn't decide that it can remove the parent node when the plugin intended to remove the node.

[danez]

The more I think about the more it makes sense what boopathi said. Though we should still have a helper for supporting plugin authors, so we can check if it is safe to remove, and some way that the plugin knows what are valid replacement options. Not sure though how this should look like.

I think this would also not be a breaking change as the plugin already throws in this cases.

[danez]

I close this for now, as I cannot come up with a solution right now.
I will open a separate PR for the change to output
() => {} instead of () => void 0 when removing the body.