ast(parser): add #456

dhruvdutt · 2018-05-20T17:38:14Z

What kind of change does this PR introduce?
Feature

Did you add tests for your changes?
Yes

If relevant, did you update the documentation?
No

Summary
Adds AST support for add command.

Create/merge logic for update AST node with add AST node
Add support for all nested node types
For nested objects, only update the required key, not replace the entire object (keep existing nested object keys unaffected)
Add tests

Does this PR introduce a breaking change?
No

evenstensberg · 2018-05-20T17:39:00Z

Split these up to PR's for Add, update and remove respectively.

evenstensberg · 2018-05-20T17:40:09Z

packages/utils/recursive-parser.js

 	if (node.size() !== 0) {
-		// push to existing key
-		return ast;
+		// select node with existing key


Abstract this

Updated to use findRootNodesByName util method.

evenstensberg · 2018-05-20T17:40:30Z

packages/utils/recursive-parser.js

+						)
+					);
+				} else if (action === "remove") {
+					j(p).remove();


Not enough, just worry about the update feature for now

ematipico · 2018-05-21T06:03:48Z

packages/utils/recursive-parser.js

+		return utils.findRootNodesByName(
+			j, root, key
+		)
+			.forEach(p => {


What is p? Could we choose a better name?

p refers to Path or NodePaths. Using variable p is a common convention with jscodeshift codemods. And we use it everywhere. A similar conventions goes for variable j which refers to jscodeshift API.

I do believe we can have a better name. @ev1stensberg WDYT? Should we change this to something like path everywhere?

But p has a different meaning based on where it is used. We have JSDoc that helps us already. At least let's give something to the developers, so when they land to the code they know what that variable is meant for.

Here's it could stay, but inside the function updateProperty, it could have a name that means something. This is my opinion.

p is alright

ematipico · 2018-05-21T06:05:35Z

packages/utils/recursive-parser.js

+	// get module.exports prop
+	const root = ast
+		.find(j.ObjectExpression)
+		.filter(p => {


ematipico · 2018-05-21T06:06:34Z

packages/utils/ast-utils.js

+ * @returns {Node} - the updated ast
+ */
+
+function updateProperty(j, p, key, value) {


Also here, p means nothing. We could call it propertyToAdd

p is fine, it's an ast node, we don't really care about the type of the node as long as we're updating its properties ( see the name of the function )

ematipico · 2018-05-21T06:07:43Z

packages/utils/ast-utils.js

+ */
+
+function updateProperty(j, p, key, value) {
+	let valForNode;


valForNode can be moved after the if. There's no need to declare it if we exist from the function

I think we can move it below if (!p) { return; } check, but not inside if block as it's getting assigned in every if else condition which will make variable declarations redundant.

I think we can move it below if (!p) { return; } check

Yes, that's what I meant

First rule of recursion is to have an exit condition, but for this function it needs to exit gracefully, as it's expected to return a node

ematipico · 2018-05-21T06:09:29Z

packages/utils/ast-utils.js

+		return;
+	}
+	if (Array.isArray(value)) {
+		let arrExp = j.arrayExpression([]);


arrExp can have different name. Maybe valueForNodeAsArray? Or something like that means something

Not sure about the name. arrExp will be either a blank/new ArrayExpression node or existing ArrayExpression node in case of existing array properties like plugins. The final value (right-hand part) will be stored in valForNode variable.

Yes, I get that. I am more up for names that mean something based on the context of the operation. Just that.

Maybe a comment will do, if you follow common sense we check if the value is an array exp, so we want to create an array node

ematipico · 2018-05-21T06:11:25Z

packages/utils/ast-utils.js

+			addProperty(j, arrExp, null, val);
+		});
+		valForNode = arrExp;
+	} else if (


Could we have comment here where we explain what kind of check we're doing? That __paths sounds like an internal thing, so it would be better to explain why

@ev1stensberg This check is to avoid AST nodes during the recursion, right? So, this will pass only object values from webpack config and avoid AST node objects.

If a node has __paths, it's a regexp which cannot be resolved into a regular literal or identifier

Worth leaving a comment inside the code

ematipico · 2018-05-21T06:12:51Z

packages/utils/ast-utils.js

+	}
+	let pushVal;
+	if (key) {
+		pushVal = j.property("init", j.identifier(key), valForNode);


When do you have this case?

Almost every case. What happens is that we compute the right-hand part of a particular property (for instance development or production) and store it in valForNode variable. We then push this value to the key (mode in this case). Similarly, valForNode can be a Literal (mode), ArrayExpression (plugins) or ObjectExpression(output).

The else part of this check happens when we recursively add something. For instance, we have an array of objects (rules). We will recursively add every object to an ArrayExpression. In this case, there is no key. We just have to keep adding objects inside the array.

Thanks 😄

ematipico

Few names to change plus questions

evenstensberg · 2018-05-21T11:52:00Z

packages/utils/recursive-parser.js

+		return utils.findRootNodesByName(
+			j, root, key
+		)
+			.forEach(p => {


p is alright

evenstensberg · 2018-05-21T11:53:14Z

packages/utils/recursive-parser.js

 		return root.forEach(p => {
 			if (key === "topScope") {
 				utils.parseTopScope(j, p, value);
 			} else if (key === "merge") {
 				utils.parseMerge(j, p, value);
 			} else {
+				// init, add new property


comment right after conditional if you're gonna comment it. Also, I think it's clear what we are doing here

This part would be used in add command in cases when a property doesn't exist. For instance, when someone tries to add mode property which isn't present in config, node.size() will be 0 and this addProperty() would be called. And in cases when mode is already present and we're overwriting it by updateProperty(). Basically, add and update commands would work same.

That's why want to make super clear that this part of the code is getting used in init and add.

Maybe we can just have three options: add (renamed from init), update and remove

evenstensberg · 2018-05-21T11:54:39Z

packages/utils/ast-utils.js

+		!(value.__paths || value instanceof RegExp)
+	) {
+		let objectExp = j.objectExpression([]);
+		if (safeTraverse(p, [


You'll have to check the entire key/val operation. Not sure if this is enough, let me test

I've added some tests. Let me know if you run into any issues here.

evenstensberg · 2018-05-21T11:55:19Z

packages/utils/ast-utils.js

+ */
+
+function updateProperty(j, p, key, value) {
+	let valForNode;


First rule of recursion is to have an exit condition, but for this function it needs to exit gracefully, as it's expected to return a node

evenstensberg · 2018-05-21T11:55:45Z

packages/utils/ast-utils.js

+ * @returns {Node} - the updated ast
+ */
+
+function updateProperty(j, p, key, value) {


p is fine, it's an ast node, we don't really care about the type of the node as long as we're updating its properties ( see the name of the function )

sendilkumarn · 2018-05-21T21:10:37Z

packages/utils/__testfixtures__/fixture-2.input.js

+
+module.exports = {
+	entry: {
+        ed: 'index.js',


really ed Sheeran ha ha 😄

He's a good singer though! 😆

evenstensberg · 2018-05-22T00:04:11Z

@dhruvdutt could you do a rebase?

dhruvdutt · 2018-05-23T18:35:53Z

Rebased. PTAL @ev1stensberg

ematipico · 2018-05-24T06:12:55Z

packages/utils/recursive-parser.test.js

+				}
+			}
+		],
+		nice: "':)'",


Could you also adda a Promise to the test? Just to make sure that we cover every case

{ foo: Promise.resolve() }

Added! 👍

ematipico · 2018-05-24T06:15:17Z

packages/utils/recursive-parser.test.js

@@ -25,3 +25,55 @@ defineTest(
 	},
 	"init"
 );
+


How does it behave if you tried to add property that already exists? Could you define a test for this case please?

^ Otherwise this would just be the init feature

entry was already present in the input fixture. I've added mode as well to make it clearer.

evenstensberg · 2018-06-01T17:38:26Z

packages/utils/ast-utils.js

+/**
+ *
+ * Traverse safely and return `type` for path object with value.value property
+ * @param {Node} path - pathNode


change to AST node in description

evenstensberg · 2018-06-01T17:39:06Z

packages/utils/ast-utils.js

@@ -415,6 +444,11 @@ function addProperty(j, p, key, value) {
 	} else {
 		pushVal = valForNode;
 	}
+
+	// incase of _add_ or while updating a property,
+	// we only return the generated pushVal which will be replace the node path


keep this, discard the other comment

evenstensberg · 2018-06-01T17:39:44Z