ast(parser): command remove #461

dhruvdutt · 2018-05-23T18:09:49Z

What kind of change does this PR introduce?
Feature

Did you add tests for your changes?
No

If relevant, did you update the documentation?
No

Summary

[EDIT] Forgot to show that if there are more than one key in an object, it will keep other keys and if there is only one key in an object then it will delete the whole object.

Does this PR introduce a breaking change?
No

Closes #500

evenstensberg · 2018-05-24T05:52:39Z

packages/generators/remove-generator.js

+		return this.prompt([
+			List(
+				"actionType",
+				"What property do you want to remove?",


Which would probably be better here

evenstensberg · 2018-05-24T05:53:10Z

packages/generators/remove-generator.js

+				Array.from(PROP_TYPES.keys())
+			)
+		])
+			.then(actionTypeAnswer => {


You'll need to prompt for sub-props too. Check out how the add generator is built

I think in case of removal, sub-props would need webpack config to be read. For instance, in case of plugins, before we prompt user which plugin he wants to remove, we need to read all plugins from the webpack config file.

In case of add, it works as we prompt all options (or plugins that can be installed) and then the user selects which plugin to install.

If we want to go for sub-prompts then it would need the reading of webpack config file. That's why I've kept it simple for now and removed the key that is selected directly. WDYT?

could you try to impl the read from config? It's quite nice

evenstensberg · 2018-05-24T05:53:32Z

packages/utils/recursive-parser.js

+		)
+			.forEach(p => {
+				if (action === "remove") {
+					// remove property


needs to be recursive

evenstensberg

A few missing bits here:

Check out the add generator we have. Do the same, but ask if the user wants to remove some sub-props instead. Build up a yeoman rc-file based on the current config, but remove parts we've prompted. After doing so, you can just call something similar to utils.addProperty on the yeoman file, and this would be equivalent to removing a node.

This is rather expensive to do, so it might be better to find the node you're trying to remove and then removing it. That requires you to mark the property in the yeoman file, and it would be cumbersome developer-wise. I'd advise you to do the first mentioned thing.

This means:

Build a yeoman file.
Traverse through the ast using a similar method as utils.addProperty, but check if the node we're at is present in the yeoman config and in the webpack file. If it is, then we shouldn't do anything.
When we find a node that is in the webpack file, but not in the yo file, remove it.

webpack-bot · 2018-06-05T20:00:44Z

@dhruvdutt Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ev1stensberg Please review the new changes.

evenstensberg · 2018-06-05T21:05:43Z

packages/utils/recursive-parser.js

@@ -39,6 +39,9 @@ module.exports = function recursiveTransform(j, ast, key, value, action) {
 			if (action === "add") {
 				// update property/replace
 				j(p).replaceWith(utils.addProperty(j, p, key, value, action));
+			} else if (action === "remove") {
+				// remove property
+				j(p).remove();


is this enough?

Definitely not enough. Working on AST parser for removal..

How is it going? What have you done today?

evenstensberg · 2018-06-05T21:06:07Z

packages/generators/remove-generator.js

+			});
+	}
+
+	writing() {


Looking at this, we can abstract the add generator into a base generator and pass in the name ( action ), and use that instead if the behaviour is the same

The logic for removal is quite different and simple. It just keeps on checking type of value or right-hand part and prompts for removal of keys.
I think there is no common logic between add and remove generator other than saving of yeoman rc file inside writing().

evenstensberg

Work on the ast part + review comments

evenstensberg · 2018-06-06T22:22:42Z

packages/generators/remove-generator.js

+				if (typeof propValue === "object") {
+					if (Array.isArray(propValue)) {
+						// TODO: handle Array<string>, Array<object>
+						console.log("TODO: array type node");


evenstensberg · 2018-06-06T22:23:00Z

packages/generators/remove-generator.js

+						])
+							.then(({ keyType }) => {
+								if (propType === "module" && keyType === "rules") {
+									return this.prompt([


evenstensberg · 2018-06-15T11:47:00Z

Let's go for the recursive approach. Feel free to do a benchmark and reopen this if the non-recursive gives better performance. :)

evenstensberg

CI:

ERROR: packages/info/index.ts[8, 2]: Calls to 'console.log' are not allowed.

Fix:

https://eslint.org/docs/rules/no-console#rule-details

dhruvdutt · 2018-06-28T18:11:38Z

Actually, the tslint issue was resolved in another TS PR. Anyway, I've added the same fix to this PR as well.

The tests are failing due to other TS related reasons.

evenstensberg · 2018-06-28T22:05:03Z

packages/utils/recursive-parser.test.js

+
+defineTest(
+	__dirname,
+	"remove",


You can just do one deep test for remove with multiple props

This approach is non-recursive and it doesn't drill down deep inside props.
Recursive drilling is only needed for the creation of nodes.

We do cover every case as demonstrated in the GIF. :)
The tests have to be individual. We can add more tests if needed. I think one-two test for each prop type should be good.

I just want to see some deep prop removals succeed, like resolve.alias

evenstensberg · 2018-06-28T22:05:53Z

packages/info/index.ts


 /**
 * Prints debugging information for webpack issue reporting
 */

 export default async function info() {
-	console.log(
+	process.stdout.write(


is this still throwing console error with the rule enabled?

No, it would show the warning. We can decide on one standard convention to follow between console or process.

ematipico · 2018-06-29T18:02:41Z

@dhruvdutt you can rebase now and the TS lint fix should be ready :)

Next time, please do a cherry-pick

webpack-bot · 2018-06-29T18:30:10Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

dhruvdutt

Looks good to go.

ematipico

Amazing! Good work here!!

evenstensberg

A test with a deep property and this is lgtm from me

dhruvdutt · 2018-06-30T13:30:38Z

A test with a deep property and this is lgtm from me

I've added tests for resolve.alias property.

evenstensberg

Lgtm,

dhruvdutt self-assigned this May 23, 2018

webpack-bot added the PR: unreviewed label May 23, 2018

dhruvdutt requested review from evenstensberg and ematipico May 23, 2018 18:10

evenstensberg reviewed May 24, 2018

View reviewed changes

ematipico added the Semver: minor ⚙️ When delivering new features that don't break label May 24, 2018

evenstensberg requested changes May 30, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested PR: conflict and removed PR: unreviewed labels May 30, 2018

dhruvdutt force-pushed the delete branch from 7e7771a to bac9226 Compare June 5, 2018 20:00

webpack-bot removed the PR: conflict label Jun 5, 2018

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Jun 5, 2018

evenstensberg requested changes Jun 5, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Jun 5, 2018

evenstensberg requested changes Jun 6, 2018

View reviewed changes

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Jun 7, 2018

dhruvdutt changed the title ~~[WIP] ast(parser): remove~~ ast(parser): remove Jun 7, 2018

dhruvdutt changed the title ~~ast(parser): remove~~ [non-recursive] ast(parser): remove Jun 8, 2018

evenstensberg closed this Jun 15, 2018

dhruvdutt reopened this Jun 27, 2018

webpack-bot added the PR: conflict label Jun 27, 2018

dhruvdutt force-pushed the delete branch 2 times, most recently from 167599e to b9074a8 Compare June 27, 2018 14:44

webpack-bot removed the PR: conflict label Jun 27, 2018

dhruvdutt force-pushed the delete branch from 7b21b70 to d13e6b2 Compare June 27, 2018 15:22

dhruvdutt mentioned this pull request Jun 27, 2018

[recursive] ast(parser): remove #500

Closed

This comment has been minimized.

Sign in to view

evenstensberg requested changes Jun 28, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Jun 28, 2018

dhruvdutt force-pushed the delete branch from d13e6b2 to 900671d Compare June 28, 2018 18:09

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Jun 28, 2018

evenstensberg reviewed Jun 28, 2018

View reviewed changes

ast(parser): remove

85e1e7b

dhruvdutt force-pushed the delete branch from 900671d to 85e1e7b Compare June 29, 2018 18:18

dhruvdutt changed the title ~~[non-recursive] ast(parser): remove~~ ast(parser): remove Jun 29, 2018

dhruvdutt changed the title ~~ast(parser): remove~~ ast(parser): command remove Jun 29, 2018

webpack-bot added the PR: CI-ok label Jun 29, 2018

dhruvdutt commented Jun 29, 2018

View reviewed changes

ematipico approved these changes Jun 30, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Jun 30, 2018

evenstensberg requested changes Jun 30, 2018

View reviewed changes

webpack-bot added the PR: reviewed-changes-requested label Jun 30, 2018

evenstensberg approved these changes Jun 30, 2018

View reviewed changes

dhruvdutt removed the PR: reviewed-changes-requested label Jun 30, 2018

dhruvdutt merged commit faeec57 into webpack:next Jun 30, 2018

dhruvdutt deleted the delete branch June 30, 2018 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ast(parser): command remove #461

ast(parser): command remove #461

dhruvdutt commented May 23, 2018 •

edited

Loading

evenstensberg May 24, 2018

ematipico May 24, 2018

evenstensberg May 24, 2018

dhruvdutt May 24, 2018 •

edited

Loading

evenstensberg May 24, 2018

evenstensberg May 24, 2018

evenstensberg left a comment •

edited

Loading

webpack-bot commented Jun 5, 2018

evenstensberg Jun 5, 2018

dhruvdutt Jun 6, 2018

evenstensberg Jun 6, 2018

evenstensberg Jun 5, 2018

dhruvdutt Jun 6, 2018

evenstensberg Jun 6, 2018

evenstensberg left a comment

evenstensberg Jun 6, 2018

evenstensberg Jun 6, 2018

evenstensberg commented Jun 15, 2018

This comment has been minimized.

evenstensberg left a comment

dhruvdutt commented Jun 28, 2018 •

edited

Loading

evenstensberg Jun 28, 2018

dhruvdutt Jun 29, 2018

evenstensberg Jun 29, 2018

evenstensberg Jun 28, 2018

dhruvdutt Jun 29, 2018

ematipico commented Jun 29, 2018

webpack-bot commented Jun 29, 2018

dhruvdutt left a comment

ematipico left a comment

evenstensberg left a comment

dhruvdutt commented Jun 30, 2018

evenstensberg left a comment

ast(parser): command remove #461

ast(parser): command remove #461

Conversation

dhruvdutt commented May 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvdutt May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evenstensberg left a comment • edited Loading

Choose a reason for hiding this comment

webpack-bot commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evenstensberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evenstensberg commented Jun 15, 2018

This comment has been minimized.

evenstensberg left a comment

Choose a reason for hiding this comment

dhruvdutt commented Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Jun 29, 2018

webpack-bot commented Jun 29, 2018

dhruvdutt left a comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

evenstensberg left a comment

Choose a reason for hiding this comment

dhruvdutt commented Jun 30, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

dhruvdutt commented May 23, 2018 •

edited

Loading

dhruvdutt May 24, 2018 •

edited

Loading

evenstensberg left a comment •

edited

Loading

dhruvdutt commented Jun 28, 2018 •

edited

Loading