Add initial support for --dry-run #58

aulisius · 2018-03-25T11:29:21Z

Still unsure how to show the actual diff or what kind of tests to write for this. Currently, I've used difflet package but not sure if that is enough.

Closes #6

aulisius · 2018-03-26T01:36:06Z

Added a better differ.

The output looks something like this right now

Before upgrade
    "babel-cli": "^6.26.0",
    "babel-core": "^6.26.0",
    "babel-loader": "^7.1.1",
    "babel-plugin-transform-react-jsx-self": "^6.22.0",
    "babel-plugin-transform-react-jsx-source": "^6.22.0",
    "babel-plugin-transform-runtime": "^6.23.0",
    "babel-polyfill": "^6.26.0",
    "babel-preset-env": "^1.6.0",
    "babel-preset-react": "^6.24.1",
    "babel-preset-stage-2": "^6.24.1",

After upgrade
    "@babel/cli": "7.0.0-beta.40",
    "@babel/core": "7.0.0-beta.40",
    "@babel/plugin-transform-react-jsx-self": "7.0.0-beta.40",
    "@babel/plugin-transform-react-jsx-source": "7.0.0-beta.40",
    "@babel/plugin-transform-runtime": "7.0.0-beta.40",
    "@babel/polyfill": "7.0.0-beta.40",
    "@babel/preset-env": "7.0.0-beta.40",
    "@babel/preset-react": "7.0.0-beta.40",
    "@babel/preset-stage-2": "7.0.0-beta.40",
    "babel-loader": "^8.0.0-beta.0",

noahlemen

looking good to me! will leave some more thoughts later when i've got a chance

do you have any thoughts on testing this? it would be nice if we could assert that we don't write anything when a dry run happens

also, any thoughts on the flag? as mentioned here, it might be safer to flip this and use --write as the flag that does write changes

noahlemen · 2018-03-26T16:35:58Z

src/index.js

-    pkg.babel = upgradeConfig(pkg.babel, options);
+    updatedPkg.babel = upgradeConfig(pkg.babel, options);
+  }
+  if (options.dryRun) {


maybe this block should happen whether or not we're doing a dry run? eg, have dry run only control whether writeJsonFile gets called

hzoo · 2018-03-26T16:45:48Z

Just wondering, is there a way to show the diff like in git? like with + and -? or like jest snapshots?

I think showing the files one after the other would be difficult to read? Wonder if we could just render a github type diff on a website even. Although maybe I'd just tell them to run it and run git diff themselves 😄

aulisius · 2018-03-27T15:38:44Z

@hzoo Yup, the diff package supports that. The current output looks something like this.

Index: package.json
===================================================================
--- package.json        Before Upgrade
+++ package.json        After Upgrade
@@ -62,23 +62,23 @@
     "string-format": "^0.5.0",
     "traverse": "^0.6.6"
   },
   "devDependencies": {
+    "@babel/cli": "7.0.0-beta.40",
+    "@babel/core": "7.0.0-beta.40",
+    "@babel/plugin-transform-react-jsx-self": "7.0.0-beta.40",
+    "@babel/plugin-transform-react-jsx-source": "7.0.0-beta.40",
+    "@babel/plugin-transform-runtime": "7.0.0-beta.40",
+    "@babel/polyfill": "7.0.0-beta.40",
+    "@babel/preset-env": "7.0.0-beta.40",
+    "@babel/preset-react": "7.0.0-beta.40",
+    "@babel/preset-stage-2": "7.0.0-beta.40",
     "autoprefixer": "8.1.0",
-    "babel-cli": "^6.26.0",
-    "babel-core": "^6.26.0",
     "babel-eslint": "^7.1.0",
-    "babel-loader": "^7.1.1",
+    "babel-loader": "^8.0.0-beta.0",
     "babel-plugin-lodash": "^3.3.2",
     "babel-plugin-transform-imports": "^1.4.1",
-    "babel-plugin-transform-react-jsx-self": "^6.22.0",
-    "babel-plugin-transform-react-jsx-source": "^6.22.0",
     "babel-plugin-transform-react-remove-prop-types": "^0.4.10",
-    "babel-plugin-transform-runtime": "^6.23.0",
-    "babel-polyfill": "^6.26.0",
-    "babel-preset-env": "^1.6.0",
-    "babel-preset-react": "^6.24.1",
-    "babel-preset-stage-2": "^6.24.1",
     "bitbar-webpack-progress-plugin": "^0.1.3",
     "clean-webpack-plugin": "^0.1.17",
     "copy-webpack-plugin": "^4.5.0",
     "css-loader": "^0.28.9",

@kedromelon I'm not really sure about the tests myself, as I don't have enough experience writing them. Would love more input on it.

aulisius · 2018-03-30T19:41:20Z

Rolled back some of my changes and found a simpler way of showing the diff. Following your review @kedromelon, the diff shows up always and only the write action is controlled by the --dry-run flag.

Still unsure of how a test would look like. Will think about it over the weekend.

noahlemen

@aulisius changes look fantastic, definitely an improvement in clarity!

i'm still thinking about how best to do tests here as well...

noahlemen · 2018-03-31T23:49:38Z

src/index.js


-  await writeJsonFile(path, pkg, { detectIndent: true });
+  if (!options.dryRun) {
+    await writeJsonFile(path, pkg, { detectIndent: true });


for testing this, perhaps we could stub the default export of write-json-file with jest and then assert that it is called (with the correct arguments) when options.dryRun is true and assert that it is not called when options.dryRun isn't?

I was thinking along the same lines. I can get started on that. What about the diffs? Is there any value to testing the diff output itself?

…/babel-upgrade into feat/add-dry-run-option

ljharb · 2018-04-01T14:19:49Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "babel-upgrade",
-  "version": "0.0.15",
+  "version": "0.0.16",


Typically version numbers should never be bumped in PRs (or at least, not in PRs that do anything else)

Forgot to rebase before pushing, the master branch actually has version as 0.0.16 not sure why the diff isn't updated with master.

ah i was just looking at the "view changes" diff, no worries

aulisius · 2018-04-04T16:18:52Z

@hzoo @kedromelon Added a test. Please do tell if that is enough.

noahlemen · 2018-04-04T17:41:55Z

__tests__/packageData.test.js

@@ -74,3 +74,21 @@ test('jest babel-core bridge', async () => {
 test('webpack v1 compatibility', async () => {
  expect(await updatePackageJSON(webpackV1Fixture)).toMatchSnapshot();
 });
+
+test('respects dry run', async () => {


this is great! thanks for adding!!

just two suggestions:

could we make __tests__/index.test.js and do this test there?

could we also test that writeJsonFile is called when the dry run is not used?

aulisius · 2018-04-05T03:55:58Z

Updated the tests. I think the last thing to discuss is with regards to the flag. Do we make --dry-run the default or leave things as it is?

futagoza · 2018-04-05T14:13:01Z

How about:

By default, the CLI does the dry-run and shows the diff, then asks if it should apply the changes
With the --dry-run or -d flags supplied, the CLI exits after showing the diff
With the --apply or -a flags the CLI will apply the upgrade and show the diff then

By default this avoids surprises, helps to review changes and involves the upgrader more.
The flags help in automating tasks (scripts, watchers, ci, etc)

Also, if it hasn't been implemented (sorry, just found this repo so haven't had a chance to dig into the codebase), how about adding a cache system (mtime?). This would be very helpful on monorepos 😄

noahlemen

awesome! tests look great. regarding flag itself and its defaults i think it does make sense to dry run by default, and require a --write flag to allow it to write, as discussed in #6

if we are to do that, however, we need to be sure to update the readme with this new detail. given that, i think it would be OK to have this PR just add the support for the functionality, and let the behavior change and documentation updates be handled in a separate PR? open to whatever though, curious to hear others' thoughts!

aulisius · 2018-04-09T00:34:39Z

Made --dry-run the default. User would have to pass --write to apply the changes.

@futagoza I do like your suggestion but I think it can be done as part of another PR because it requires quite some changes.

@kedromelon Yea, I also think it'd be better if those documentation and/or README updates happened in another PR.

as discussed in #58 (comment), `babel-upgrade` will make no changes unless a `--write` or `-w` flag is provided after #58 is merged. this PR updates the readme to communicate this change in functionality.

noahlemen · 2018-04-09T00:50:41Z

@aulisius awesome! started #62 to continue coordinating the documentation aspect of this change.

noahlemen · 2018-04-09T01:21:02Z

src/bin.js

@@ -38,7 +50,7 @@ async function hasFlow() {
  await writePackageJSON(upgradeOptions);

  // TODO: add smarter CLI option handling if we support more options
-  if (process.argv[2] === '--install') {
+  if (cliOptions.write && cliOptions.installDeps) {


thought of this after your comment on #62

do you think we should provide a warning when the user gives the --install flag but does not include --write? otherwise it might be difficult for them to understand what is going on in that situation?

Yeah, agreed. Will add that

aulisius · 2018-04-09T02:05:20Z

Added the note about install w/o write! @kedromelon we're good to go.

* add --write flag to readme (followup for #58) as discussed in #58 (comment), `babel-upgrade` will make no changes unless a `--write` or `-w` flag is provided after #58 is merged. this PR updates the readme to communicate this change in functionality.

ctsstc · 2018-08-14T23:33:52Z

Is this in v7 yet? I'm trying to do the dry run flag but I still get files generated. I'm using Babel 7.0.0-rc.1 I also didn't notice anything in my --help flag when running ./node_modules/.bin/babel src --help

I'm trying to see the plugins and target browsers being used when debug is set to true. Is there a better way to do this or a more direct command for such? I know npx browserslist will show you what your target browsers are if you are using a .browserslistrc file. But I would also like to see the plugins/polyfills being used given the target browsers.

faizaan.m added 2 commits March 25, 2018 16:56

Add initial support for --dry-run

29853f1

Use a better differ

4d1e53e

faizaan.m and others added 2 commits March 26, 2018 11:36

Fix undefined error

1b7c8c9

0.0.16

a9af629

noahlemen added the enhancement New feature or request label Mar 26, 2018

noahlemen reviewed Mar 26, 2018

View reviewed changes

Update diff view

7ad039f

Simplify showing diff

31234bc

noahlemen reviewed Mar 31, 2018

View reviewed changes

Remove redundant JSON.parse

f169c39

noahlemen reviewed Mar 31, 2018

View reviewed changes

faizaan.m added 7 commits April 1, 2018 18:55

Add initial support for --dry-run

925eebc

Use a better differ

cca4502

Fix undefined error

afb8fde

Update diff view

8850bb6

Simplify showing diff

b8b5570

Remove redundant JSON.parse

d1f5369

Merge branch 'feat/add-dry-run-option' of https://github.com/aulisius…

558a9fb

…/babel-upgrade into feat/add-dry-run-option

ljharb reviewed Apr 1, 2018

View reviewed changes

faizaan.m added 2 commits April 4, 2018 21:39

Add tests when dryRun is true

2c80782

Use fixtures for babelrc and mocha

a23ad83

noahlemen reviewed Apr 4, 2018

View reviewed changes

Move tests to new file

97e17b6

noahlemen approved these changes Apr 5, 2018

View reviewed changes

Make dry-run the default.

69aa45d

Object.assign > spread

6f654d5

noahlemen mentioned this pull request Apr 9, 2018

add --write flag to readme (followup for #58) #62

Merged

noahlemen reviewed Apr 9, 2018

View reviewed changes

Add warning about --install w/o --write

41b4eba

noahlemen merged commit e72f80c into babel:master Apr 9, 2018

aulisius deleted the feat/add-dry-run-option branch April 9, 2018 23:28

noahlemen mentioned this pull request Apr 23, 2018

stub console.log and console.warn in index.test.js #64

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial support for --dry-run #58

Add initial support for --dry-run #58

aulisius commented Mar 25, 2018

aulisius commented Mar 26, 2018

noahlemen left a comment

noahlemen Mar 26, 2018

hzoo commented Mar 26, 2018

aulisius commented Mar 27, 2018

aulisius commented Mar 30, 2018

noahlemen left a comment

noahlemen Mar 31, 2018

aulisius Apr 1, 2018

ljharb Apr 1, 2018 •

edited

Loading

aulisius Apr 1, 2018

ljharb Apr 1, 2018

aulisius commented Apr 4, 2018

noahlemen Apr 4, 2018 •

edited

Loading

aulisius commented Apr 5, 2018

futagoza commented Apr 5, 2018 •

edited

Loading

noahlemen left a comment •

edited

Loading

aulisius commented Apr 9, 2018

noahlemen commented Apr 9, 2018 •

edited

Loading

noahlemen Apr 9, 2018 •

edited

Loading

aulisius Apr 9, 2018

aulisius commented Apr 9, 2018

ctsstc commented Aug 14, 2018 •

edited

Loading

Add initial support for --dry-run #58

Add initial support for --dry-run #58

Conversation

aulisius commented Mar 25, 2018

aulisius commented Mar 26, 2018

noahlemen left a comment

Choose a reason for hiding this comment

noahlemen Mar 26, 2018

Choose a reason for hiding this comment

hzoo commented Mar 26, 2018

aulisius commented Mar 27, 2018

aulisius commented Mar 30, 2018

noahlemen left a comment

Choose a reason for hiding this comment

noahlemen Mar 31, 2018

Choose a reason for hiding this comment

aulisius Apr 1, 2018

Choose a reason for hiding this comment

ljharb Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

aulisius Apr 1, 2018

Choose a reason for hiding this comment

ljharb Apr 1, 2018

Choose a reason for hiding this comment

aulisius commented Apr 4, 2018

noahlemen Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

aulisius commented Apr 5, 2018

futagoza commented Apr 5, 2018 • edited Loading

noahlemen left a comment • edited Loading

Choose a reason for hiding this comment

aulisius commented Apr 9, 2018

noahlemen commented Apr 9, 2018 • edited Loading

noahlemen Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

aulisius Apr 9, 2018

Choose a reason for hiding this comment

aulisius commented Apr 9, 2018

ctsstc commented Aug 14, 2018 • edited Loading

ljharb Apr 1, 2018 •

edited

Loading

noahlemen Apr 4, 2018 •

edited

Loading

futagoza commented Apr 5, 2018 •

edited

Loading

noahlemen left a comment •

edited

Loading

noahlemen commented Apr 9, 2018 •

edited

Loading

noahlemen Apr 9, 2018 •

edited

Loading

ctsstc commented Aug 14, 2018 •

edited

Loading