[recursive] ast(parser): remove #500
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lerna workspace
* fix(loader,plugin): fix generators path bug * cli(refactor): improve folder structure (webpack#371) * cli(refactor): improve folder structure * chore(linting): fix linter errors * cli(filepath): use local import instead * cli(migrate): refactor error handling * chore(review): fix review comments * chore(review): fix review comments * chore(review): fix review comments * chore(version): v.2.0.14 * Add break as commit type and listed the type of commit in the documentation (webpack#379) * docs(commits): listed the list of type of commits available * cli(init): mode support to config (webpack#364) * cli(init): mode support to config * cli(bugfix): Allow mode "none" in CLI (webpack#381) * cli(init): use extractMiniCSSPlugin (webpack#363) * cli(init): skip redundant question * cli(init): use mini-css-extract-plugin * Retrieve information for CLI option from webpack schema options file (webpack#392) * cli(refactor): fetch available modes directly from webpack options schema * cli(refactor): Retrieve information from webpackOptionSchema * cli(add): write configuration to yeoman file (webpack#348) * misc(add): variable parity, prettify * cli(add): write config to yeoman-rc * misc(add): improve generator questions * fix(init): output file name for single output (webpack#403) improved output filename * cli(migrate): Update migration question (webpack#402) * cli(init): webpack4 ready (webpack#356) * cli(init): webpack4 ready * cli(init): remove unused variable, still @next on etwp * cli(init): Allow to use default entry in `init` * cli(init): Fix typo in comment * cli(init): Optimization transform and tests * cli(init): Fix non-optimized option for splitChunks * cli(init): Add cachingGroup per entry, don't show name in prod * cli(init): Add cachingGroup's defaults, fix entry * cli(init): Add a link to where the defaults live * cli(init): Remove default caching group definition from example * Add NoEmitOnErrorsPlugin transformation (webpack#399) * ast(migrate): handle no emit on error * ast(migrate): handle module concatenation and named modules * ast(migrate): handle module concatenation and named modules * fix(ast): checks validity of an identifier (webpack#360) added test cases for validate identifier * cli(entry): quotes sanitization (webpack#337) * chore: minor doc fixes * cli(entry): quotes sanitization * tests(entry): add test case for double quotes * tests(entry): update snapshots * misc(utils): entry - variable parity * misc(prop-types): sort * cli(entry): multiple entries quotes sanitization * chore(versioning): v 2.0.15 * chore(versioning): push new package version * cli(fix): remove reference to specific version during migration (webpack#410) * fix: remove reference to specific version during migration * fix: rephrase update message * chore(docs): updated old references to the extract text plugin (webpack#412) * chore(dev): added break to the list of type of commit * init(fix): removed references to extract text plugin * fix(commit): rollback * cli(migration): update UglifyJS transformation (webpack#416) * cli(migration): Update UglifyJS migration file to fit webpack4 configuration * cli(migration): Add cases where require variable does not need to exist * cli(migration): Add transformation for usage of webpack.otimization.UglifyPlugin * cli(tests): Update test snapshots after updating transformation * cli(fix): fix expressionContent being null * cli(refactor): remove plugins array if empty Created function on ast-utils so every other transformation can use this. * tests: add tests for new ast-utils method * fix: fix test names and jsdoc * fix: update maxSize for utils folder * ast(cli): Recursively parse AST (webpack#341) * ast(refactor): wip refactor * ast(refactor): wip refactor * ast(init): refactor * test(refactor): refactor test suite * tests(define): swap args * ast(parsing): refactor stuff * ast(init): refactor * ast(init): refactor tests * chore(tests): remove some unneeded tests * chore(pkg): update package.json * chore(project): clear up project structure * chore(cli): remove unneded files * chore(git): add gitignore to yeoman file * chore(deps): update pkg.json * tests(snapshots): update snapshots * tests(jest): use empty module for snapshots * tests(snap): only test one prop * chore(publishing): add semantic-release (webpack#415) * chore(release): [WIP] add semantic-release * test(ci): wip * test(ci): add node versions * test(ci): remove extra test * tests(ci): revise * tests(ci): only push to npm on master * tests(ci): use matrix on jobs * tests(ci): revise * tests(ci): update * tests(ci): test * tests(ci): test * tests(ci): p * tests(ci): update travis.yml * chore(release): revise travis * misc(yeoman): update yeoman * chore(travis): revise travis * misc(travis): revise travis.yml * chore(travis): remove redundant code * chore(pkg): add keywords * 2.1.1 * 2.1.2 * cli(cmds): revise yargs command (webpack#422) * use yargs.command instead of yargs.option for sub-commands * cli(node): Add node flags to CLI (webpack#377) * feat: add support for node flags * tests: Fix node-flags test * misc: Fix test failing due to not-found webpack-cli * misc: remove comment * misc: refactor removing unecessary args * tests: add more tests to prevent argument collision * cli(cmds): remove strict * fix(node): remove node option for now * chore(lerna): refactor * chore(lerna): refactor * chore(refactor): refactor stuff * chore(package): update pkg.lock * chore(templates): Update issue templates (webpack#432) Adds fancy templates * ast(init): add topScope prop * ast(merge): re-add merge prop * cli(lerna): refactor * chore(lerna): refactor * chore(lerna): update * chore(lint): revise * chore(refactor): refactor * chore(tests): use lerna for tests * chore(travis): don't cache me outside * chore(pkg): remove prefer global * chore(release): v.2.1.3 * chore(package.lock): update pkg.lock * fix(loader,plugin): fix generators path bug * chore(version): v.2.0.14 * Add break as commit type and listed the type of commit in the documentation (webpack#379) * docs(commits): listed the list of type of commits available * cli(bugfix): Allow mode "none" in CLI (webpack#381) * cli(init): use extractMiniCSSPlugin (webpack#363) * cli(init): skip redundant question * cli(init): use mini-css-extract-plugin * Retrieve information for CLI option from webpack schema options file (webpack#392) * cli(refactor): fetch available modes directly from webpack options schema * cli(refactor): Retrieve information from webpackOptionSchema * cli(add): write configuration to yeoman file (webpack#348) * misc(add): variable parity, prettify * cli(add): write config to yeoman-rc * misc(add): improve generator questions * fix(init): output file name for single output (webpack#403) improved output filename * cli(migrate): Update migration question (webpack#402) * fix(ast): checks validity of an identifier (webpack#360) added test cases for validate identifier * chore(versioning): v 2.0.15 * chore(versioning): push new package version * cli(fix): remove reference to specific version during migration (webpack#410) * fix: remove reference to specific version during migration * fix: rephrase update message * chore(docs): updated old references to the extract text plugin (webpack#412) * chore(dev): added break to the list of type of commit * init(fix): removed references to extract text plugin * fix(commit): rollback * misc(yeoman): update yeoman * cli(cmds): revise yargs command (webpack#422) * use yargs.command instead of yargs.option for sub-commands * cli(node): Add node flags to CLI (webpack#377) * feat: add support for node flags * tests: Fix node-flags test * misc: Fix test failing due to not-found webpack-cli * misc: remove comment * misc: refactor removing unecessary args * tests: add more tests to prevent argument collision * cli(cmds): remove strict * fix(node): remove node option for now * chore(templates): Update issue templates (webpack#432) Adds fancy templates * cli(lerna): refactor * chore(lerna): refactor * chore(pkg): remove prefer global * chore(travis): Add encrypted private ssh key * fix(pkg): test auto setup * cli(init): revise installation steps (webpack#441) * cli(init): revise installation steps * chore(formatting): format code * cli(tests): refactor tests * chore(travis): run lockfile cmds on tests (webpack#444) * Update dependencies to enable Greenkeeper 🌴 (webpack#443) chore(greenkeeper): Update dependencies to enable Greenkeeper 🌴 * chore(docs): update readme * chore(travis): add Node.js 10 (webpack#425) * chore(travis): move npm ci to install task (webpack#424) * chore(travis): move npm ci to install task * chore: trigger new build * chore: upgrade Node.js for Appveyor to 8 * chore: remove redundanct npm install command
dhruvdutt
commented
Jun 8, 2018
| @@ -436,6 +436,7 @@ function addProperty(j, p, key, value, action) { | |||
| }); | |||
| valForNode = objectExp; | |||
| } else { | |||
| value = `'${value}'`; | |||
There was a problem hiding this comment.
Some of the code like this is a part of PR #499. I'll rebase and cleanup once it gets merged.
There was a problem hiding this comment.
do it in this PR. Your code is obviously wrong when you see that you've changed snapshots signature.
There was a problem hiding this comment.
If we want to handle this in add generator, it's easy as we can do at when we generate/assign literal string but if we think about remove generator which is essentially copying the whole config then it won't be possible to find and reassign every string literal in the webpack config object we read from user file.
There was a problem hiding this comment.
Move this to generator, AST should not be concerned with this.
evenstensberg
requested changes
Jun 8, 2018
| @@ -436,6 +436,7 @@ function addProperty(j, p, key, value, action) { | |||
| }); | |||
| valForNode = objectExp; | |||
| } else { | |||
| value = `'${value}'`; | |||
There was a problem hiding this comment.
do it in this PR. Your code is obviously wrong when you see that you've changed snapshots signature.
packages/utils/scaffold.js
Outdated
| @@ -38,7 +38,7 @@ module.exports = function runTransform(webpackProperties, action) { | |||
| const webpackConfig = Object.keys(webpackProperties).filter(p => { | |||
| return p !== "configFile" && p !== "configPath"; | |||
| }); | |||
| const initActionNotDefined = action && action !== "init" ? true : false; | |||
| const addActionDefined = action && action === "add"; | |||
There was a problem hiding this comment.
this needs a fallback value and
it will break the init feature as well.
There was a problem hiding this comment.
Added the fallback value. Actually, we can get away without fallback value as action won't be undefined ever. If it's not equal to "add", it addActionDefined would false. We will only need fallback value if action is undefined.
Also, I don't see how init will break. It will continue to work.
packages/utils/scaffold.js
Outdated
| @@ -50,7 +50,7 @@ module.exports = function runTransform(webpackProperties, action) { | |||
| transformations.push("merge"); | |||
| } | |||
| const ast = j( | |||
| initActionNotDefined | |||
| addActionDefined | |||
There was a problem hiding this comment.
Why are you changing this btw?
There was a problem hiding this comment.
Because in cases other than add (init and remove), we're starting from "module.exports = {}" and not reading existing config. We're regenerating the whole config file by using addProperty. The generator would directly clear the things to remove and the newly generated config file would be created based on this it.
|
@dhruvdutt Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ev1stensberg Please review the new changes. |
evenstensberg
requested changes
Jun 15, 2018
| @@ -1,3 +1,119 @@ | |||
| const Generator = require("yeoman-generator"); | |||
| const path = require("path"); | |||
There was a problem hiding this comment.
const { resolve } = require('path');
| configPath = null; | ||
| // end the generator stating webpack config not found or to specify the config | ||
| } | ||
| this.webpackOptions = require(configPath); |
There was a problem hiding this comment.
This is neat, but it will throw on packages that uses plugins. Maybe a JSON serialize would be better. If you did the same approach with add, the strings from the config needs to be padded in order for the ast to construct correctly.
It's unclear what the comment is pointing to, what is it suppose to comment?
|
|
||
| getModuleLoaders() { | ||
| if (this.webpackOptions.module && this.webpackOptions.module.rules) { | ||
| return this.webpackOptions.module.rules.map(rule => rule ? rule.loader : null); |
There was a problem hiding this comment.
Add a filter here cause some props might be null and therefore we shouldn't add more than needed to the yeoman file. Don't you think?
| ]) | ||
| .then(({ propType }) => { | ||
| if (!PROP_TYPES.has(propType)) { | ||
| console.log("Invalid webpack config prop"); |
There was a problem hiding this comment.
This is weak logging, provide a more descriptive message
| } else { | ||
| // remove the complete prop object if there is only one key | ||
| if (Object.keys(this.webpackOptions[propType]).length <= 1) { | ||
| delete this.webpackOptions[propType]; |
There was a problem hiding this comment.
Lucky that we aren't in strict mode :-P
| @@ -12,29 +12,29 @@ exports[`utils addProperty add entry property using add 1`] = ` | |||
| }], | |||
|
|
|||
| man: () => duper, | |||
| objects: are, | |||
| objects: 'are', | |||
There was a problem hiding this comment.
See what I mentioned about the string stuff. This is invalid syntax for the ast construction.
packages/utils/recursive-parser.js
Outdated
| @@ -39,6 +39,8 @@ module.exports = function recursiveTransform(j, ast, key, value, action) { | |||
| if (action === "add") { | |||
| // update property/replace | |||
There was a problem hiding this comment.
should have been update/replace property
webpack-bot
added
PR: reviewed-changes-requested
and removed
PR: review-outdated
labels
dhruvdutt
force-pushed
the
next
branch
3 times, most recently
from
39a5123
to
f611c27
Compare
evenstensberg
requested changes
Jun 26, 2018
|
Closed in favour of #461. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Did you add tests for your changes?
If relevant, did you update the documentation?
Summary
Add support for remove command.
Does this PR introduce a breaking change?
Other information