Can't figure out how to make insertBefore work

[theodoreb]

I'm trying to replace some things with a for..of loop but I can't find how to make insertBefore work.

Here is an exemple of what I'm trying to do: http://astexplorer.net/#/ejSwWFTL8V/2

If I uncomment line 71 I get the error:

    at Type.Tp.assert (./node_modules/recast/node_modules/ast-types/lib/types.js:61:19)
    at getMoves (./node_modules/recast/node_modules/ast-types/lib/path.js:130:17)
    at NodePath.insertAt (./node_modules/recast/node_modules/ast-types/lib/path.js:216:20)
    at NodePath.insertBefore (./node_modules/recast/node_modules/ast-types/lib/path.js:239:28)
    at NodePath.<anonymous> (./node_modules/jscodeshift/dist/collections/Node.js:159:25)
    at __paths.forEach (./node_modules/jscodeshift/dist/Collection.js:76:36)
    at Array.forEach (native)
    at Collection.forEach (./node_modules/jscodeshift/dist/Collection.js:75:18)
    at Collection.insertBefore (./node_modules/jscodeshift/dist/collections/Node.js:156:17)
    at Collection.typedMethod [as insertBefore] (./node_modules/jscodeshift/dist/Collection.js:376:43)

Or it there is a better way to acheive what I need:

(function  () {

  $(context).find('[data-drupal-selector="edit-blocks"]').once('block-highlight').each(function () {
    var $container = $(this);
  });

  /*
  Expected:

  const $elements = $(context).find('[data-drupal-selector="edit-blocks"]').once('block-highlight');
  for (const element of $elements) {
    var $container = $(element);
  }
   */

}());

[theodoreb]

Same issue with insertAfter by the way.

[fkling]

OK, I figured out what the issue is. The problem is actually that you operating on the wrong nodes. You should not be looking for CallExpressions but for ExpressionStatements that contain a CallExpression that matches your constraints.

The reason why insertBefore and insertAfter are not working is that you are inserting nodes in invalid positions. This is the node structure (simplified):

$(context).find(...).once(...).each(...);
^----------CallExpression--------------^
^----------ExpressionStatement----------^

An expression statements always only has a single expression as a child:

{
  "type": "ExpressionStatement",
  "expression": {
    "type": "CallExpression",
    ...
  }
}

insertBefore will add a node as a sibling of CallExpression, so ExpressionStatement would have two children, which is invalid.

Another way to look at it is: Since you are trying to replace the expression to $(...).each(...) with two statements, you can only really apply this transform to expressions that are children of ExpressionStatements, i.e. you wouldn't want to apply the transform to:

$(...).each(...) + 42;

(I know that doesn't make sense, but hopefully gets the point across)

With that in mind, here is an updated version your transform: http://astexplorer.net/#/5ZHnqffF3M

---

This is not necessarily something that is easy to catch (it certainly to me a while). Here is what we could do:

1. Update all mutations methods to automatically replace the parent node if and only if

• the parent node is an ExpressionStatement
• the replacement node(s) are statements

2. Print a warning if the one tries to replace an expression with a statement (we can also point out explicitly if the parent node is an ExpressionStatement).

Originally I liked (1), but I'm afraid that it's a bit too magical and kind of misleads people into thinking that such mutations are "correct".

(2) would be more valuable from an educational perspective.

Of course we could also do both 😉

[theodoreb]

Totally makes sense :) Thank you so much. Would not have figured it out without you. I kept my selector since the one you proposed wasn't catching things I wanted to transform such as $(element).find('div').each(/* ... */).on(); I updated my code here: http://astexplorer.net/#/ejSwWFTL8V/3

2. would have help to realize I was trying to do something that doesn't make sense. Not too found of magic but maybe 1) with a warning would help.

Maybe one last question: how to remove the newlines between the stuff I added? It shows

const $elements = …;

for (let element of $elements) { … }

$element.on();

instead of

const $elements = …;
for (let element of $elements) { … }
$element.on();

Also is there a better place to ask those questions than a github issue?

[kronosX3]

Long story short:
If your insertAfter doesn't work, you probably try to append SOMETHING to node, which should have only ONE child.
Solution: take some parent node and append SOMETHING to node, which allows multiple childs.