Revert "remove uneccessary target.browsers (#1004)"

[sudo-suhas]

The last released version of babel-preset-env does not yet support loading the targets from browserslist field in package.json:

I enabled the debug output for babel-preset-env in .babelrc:

diff --git a/.babelrc b/.babelrc
index 3419dae..e4d2dd8 100644
--- a/.babelrc
+++ b/.babelrc
@@ -1,7 +1,8 @@
 {
   "presets": [
     ["env", {
-      "modules": false
+      "modules": false,
+      "debug": true
     }],
     "stage-2"
   ],

λ yarn list babel-preset-env
yarn list v1.3.2
warning ..\package.json: No license field
warning Filtering by arguments is deprecated. Please use the pattern option instead.
└─ [email protected]
Done in 0.96s

λ yarn run build
yarn run v1.3.2
warning ..\package.json: No license field
$ node build/build.js
/ building for production...babel-preset-env: `DEBUG` option

Using targets:
{} // <-Here's the problem

Modules transform: false

Using plugins:
  check-es2015-constants {}
  transform-es2015-arrow-functions {}
  transform-es2015-block-scoped-functions {}
  transform-es2015-block-scoping {}
  transform-es2015-classes {}
  transform-es2015-computed-properties {}
  transform-es2015-destructuring {}
  transform-es2015-duplicate-keys {}
  transform-es2015-for-of {}
  transform-es2015-function-name {}
  transform-es2015-literals {}
  transform-es2015-object-super {}
  transform-es2015-parameters {}
  transform-es2015-shorthand-properties {}
  transform-es2015-spread {}
  transform-es2015-sticky-regex {}
  transform-es2015-template-literals {}
  transform-es2015-typeof-symbol {}
  transform-es2015-unicode-regex {}
  transform-regenerator {}
  transform-exponentiation-operator {}
  transform-async-to-generator {}
  syntax-trailing-function-commas {}
Hash: e84f4e64e9615d747221
Version: webpack 3.8.1
Time: 6312ms
                                                  Asset       Size  Chunks             Chunk Names
               static/js/vendor.4a7dfbcc057d41e2cd2b.js    94.3 kB       0  [emitted]  vendor
                  static/js/app.74a33058aa6f2da49a41.js    11.5 kB       1  [emitted]  app
             static/js/manifest.221cbdbf460b27ff90d1.js    1.51 kB       2  [emitted]  manifest
    static/css/app.d90fac408992be41cff8ba63f004fedd.css  432 bytes       1  [emitted]  app
static/css/app.d90fac408992be41cff8ba63f004fedd.css.map  828 bytes          [emitted]
           static/js/vendor.4a7dfbcc057d41e2cd2b.js.map     769 kB       0  [emitted]  vendor
              static/js/app.74a33058aa6f2da49a41.js.map    37.7 kB       1  [emitted]  app
         static/js/manifest.221cbdbf460b27ff90d1.js.map    14.4 kB       2  [emitted]  manifest
                                             index.html  517 bytes          [emitted]

  Build complete.

  Tip: built files are meant to be served over an HTTP server.
  Opening index.html over file:// won't work.

Done in 10.29s.

output after adding targets

λ yarn run build
yarn run v1.3.2
warning ..\package.json: No license field
$ node build/build.js
/ building for production...babel-preset-env: `DEBUG` option

Using targets:
{
  "chrome": "60",
  "android": "4.4.3",
  "edge": "15",
  "firefox": "56",
  "ie": "10",
  "ios": "10.3",
  "safari": "10.1"
}

Modules transform: false

Using plugins:
  check-es2015-constants {"android":"4.4.3","ie":"10"}
  transform-es2015-arrow-functions {"android":"4.4.3","ie":"10"}
  transform-es2015-block-scoped-functions {"android":"4.4.3","ie":"10"}
  transform-es2015-block-scoping {"android":"4.4.3","ie":"10"}
  transform-es2015-classes {"android":"4.4.3","ie":"10"}
  transform-es2015-computed-properties {"android":"4.4.3","ie":"10"}
  transform-es2015-destructuring {"android":"4.4.3","edge":"15","ie":"10"}
  transform-es2015-duplicate-keys {"android":"4.4.3","ie":"10"}
  transform-es2015-for-of {"android":"4.4.3","ie":"10"}
  transform-es2015-function-name {"android":"4.4.3","edge":"15","ie":"10"}
  transform-es2015-literals {"android":"4.4.3","ie":"10"}
  transform-es2015-object-super {"android":"4.4.3","ie":"10"}
  transform-es2015-parameters {"android":"4.4.3","ie":"10"}
  transform-es2015-shorthand-properties {"android":"4.4.3","ie":"10"}
  transform-es2015-spread {"android":"4.4.3","ie":"10"}
  transform-es2015-sticky-regex {"android":"4.4.3","ie":"10"}
  transform-es2015-template-literals {"android":"4.4.3","ie":"10"}
  transform-es2015-typeof-symbol {"android":"4.4.3","ie":"10"}
  transform-es2015-unicode-regex {"android":"4.4.3","ie":"10"}
  transform-regenerator {"android":"4.4.3","ie":"10"}
  transform-exponentiation-operator {"android":"4.4.3","ie":"10"}
  transform-async-to-generator {"android":"4.4.3","ie":"10"}
  syntax-trailing-function-commas {"android":"4.4.3","ie":"10"}
Hash: e84f4e64e9615d747221
Version: webpack 3.8.1
Time: 6234ms
                                                  Asset       Size  Chunks             Chunk Names
               static/js/vendor.4a7dfbcc057d41e2cd2b.js    94.3 kB       0  [emitted]  vendor
                  static/js/app.74a33058aa6f2da49a41.js    11.5 kB       1  [emitted]  app
             static/js/manifest.221cbdbf460b27ff90d1.js    1.51 kB       2  [emitted]  manifest
    static/css/app.d90fac408992be41cff8ba63f004fedd.css  432 bytes       1  [emitted]  app
static/css/app.d90fac408992be41cff8ba63f004fedd.css.map  828 bytes          [emitted]
           static/js/vendor.4a7dfbcc057d41e2cd2b.js.map     769 kB       0  [emitted]  vendor
              static/js/app.74a33058aa6f2da49a41.js.map    37.7 kB       1  [emitted]  app
         static/js/manifest.221cbdbf460b27ff90d1.js.map    14.4 kB       2  [emitted]  manifest
                                             index.html  517 bytes          [emitted]

  Build complete.

  Tip: built files are meant to be served over an HTTP server.
  Opening index.html over file:// won't work.

Done in 9.67s.

However, the targets can be removed from .babelrc once @babel/preset-env is out of beta.

[LinusBorg]

Strange, I could swear they already fixed this. It also says so in their README:

By default, @babel/preset-env will use browserslist config sources [which are:]

1. Tool options. For example browsers option in Autoprefixer.
2. BROWSERSLIST environment variable.
3. browserslist config file in current or parent directories.
4. browserslistrc config file in current or parent directories.
5. browserslist key in package.json file in current or parent directories.
6. If the above methods did not produce a valid result Browserslist will use defaults: > 1%, last 2 versions, Firefox ESR.

and there's no open issue about this that I can find.

[sudo-suhas]

The original PR is this one - babel/babel-preset-env#161 and as you can see in babel/babel-preset-env#161 (comment), the feature was landed in 2.0.0-alpha.19 but later they moved it to the monorepo and switched to that version(v7.0.0-beta.32 is the latest). But yea.. some confusing changes. For example babel-plugin-transform-es2015-modules-commonjs has been renamed and moved to @babel/plugin-transform-modules-commonjs. I expect they will ease the transition with codemods. In any case, the last released stable version of babel-preset-env does not load the targets from package.json.

[LinusBorg]

Hm, that's really inconvenient. We need browserslist in package.json for autoprefixer, for example.

So the middle ground would probably be to import it into babelrc, so we only have to manage it in one place.

Would you adjust the PR in that regard?

[LinusBorg]

Ah damn, babelrc is json, not js....

[jarrodldavis]

Once Babel 7 lands, you can use a .babelrc.js file if you need to: babel/babel#4892

Here's a workaround for 6.x in the meantime.

[sudo-suhas]

Thanks @jarrodldavis, that's very helpful.

@LinusBorg I agree that having to define the targets in 2 places is not ideal. However, this issue will be fixed once babel packages are out of beta which I am guessing will be soon. Are you sure you want me to setup .babelrc.js ?

[LinusBorg]

Hm, I'm a bit torn. But I think this could be advantagous.

Once we have a babelrc.js (even with this "hack" for the time being, until babel v7), we could actually comment the babel config and give some hints about what do do to e.g. add polyfills or whatever.

So yeah, let's actually do this.

[sudo-suhas]

This approach breaks jest 😞

λ yarn run unit
yarn run v1.3.2
warning ..\package.json: No license field
$ jest --config test/unit/jest.conf.js --coverage
 FAIL  test\unit\specs\HelloWorld.spec.js
  ● Test suite failed to run

    E:\Projects\experiments\vue-webpack-dev\test\unit\setup.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import Vue from 'vue';
                                                                                             ^^^^^^

    SyntaxError: Unexpected token import

      at ScriptTransformer._transformAndBuildScript (node_modules/jest-runtime/build/script_transformer.js:305:17)
          at Generator.next (<anonymous>)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        0.872s, estimated 1s
Ran all test suites.
Running coverage on untested files...
    Failed to collect coverage from E:\Projects\experiments\vue-webpack-dev\src\components\HelloWorld.vue
    ERROR: TypeError: Cannot read property 'fileCoverage' of undefined
    STACK: TypeError: Cannot read property 'fileCoverage' of undefined
    at Instrumenter.instrumentSync (E:\Projects\experiments\vue-webpack-dev\node_modules\istanbul-lib-instrument\dist\instrumenter.js:147:40)
    at exports.default (E:\Projects\experiments\vue-webpack-dev\node_modules\jest-cli\build\generate_empty_coverage.js:34:18)
    at module.exports (E:\Projects\experiments\vue-webpack-dev\node_modules\jest-cli\build\reporters\coverage_worker.js:52:94)
    at handle (E:\Projects\experiments\vue-webpack-dev\node_modules\worker-farm\lib\child\index.js:44:8)
    at process.<anonymous> (E:\Projects\experiments\vue-webpack-dev\node_modules\worker-farm\lib\child\index.js:51:3)
    at emitTwo (events.js:126:13)
    at process.emit (events.js:214:7)
    at emit (internal/child_process.js:772:12)
    at _combinedTickCallback (internal/process/next_tick.js:141:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)

----------|----------|----------|----------|----------|----------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------|----------|----------|----------|----------|----------------|
All files |        0 |        0 |        0 |        0 |                |
 App.vue  |        0 |        0 |        0 |        0 |... 19,20,21,22 |
----------|----------|----------|----------|----------|----------------|
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[jarrodldavis]

Hmm, since babel-jest does its own .babelrc resolution, I wonder if the relative path is an issue. Try adding a temporary console.log to .babelrc.js to see if it's even being loaded during testing?

[sudo-suhas]

Tried it, it does not get loaded during testing.

[jarrodldavis]

Hmm, looks like Jest only looks at env -> test under .babelrc, and even then using the .babelrc.js preset trick doesn't work.

[jarrodldavis]

Okay, I've figured it out.

.babelrc:

{
  "presets": [
    ["./.babelrc.js"]
  ],
  "env": {
    "test": {
      "presets": ["./.babelrc.js"]
    }
  }
}

.babelrc.js:

module.exports = function () {
  if (process.env.NODE_ENV === 'test') {
    return {
      presets: ['env', 'stage-2']
    }
  } else {
    return {
      presets: [
        ['env', {
          modules: false
        }],
        'stage-2'
      ],
      plugins: ['transform-runtime']
    }
  }
}

I also ran jest with --no-cache (as recommended here) to make sure the transformation was fresh.

[sudo-suhas]

Thanks @jarrodldavis, that does work. A little concerned about the increased complexity though.

[LinusBorg]

Yes, the added complexity is sub-optimal.

Having two places to edit broswerslist in is sub-optimal as well, especially if we can't add comments in neither of the two places since both are JSON.

So I'd say it's a trade-off that's worth it. We just have to Mae sure to make it worth to the users by offering a well commented babelrc.js

[LinusBorg]

@sudo-suhas So I've checked this out and I'm sorry to say that I think I've changed my mind again :(

I'm a fraid that the new format of the babelrc.js file with the conditional return depending on environment will lead to a lot of confusion for people not familiar with the reasons we did it.

So I guess we will have to bite the bullet and revert to having the browserslist in both package.json and .babelrc, and make a note in the docs to make people aware that they have to make changes to browserlist in two places until Babel 7 has landed.

Would you agree?

[sudo-suhas]

I'm sorry to say that I think I've changed my mind again :(

No problem at all 😄. I honestly appreciate the thought you and the rest of Vue.js team put into maintaining and developing the ecosystem 🙇.

I'm afraid that the new format of the babelrc.js file with the conditional return depending on environment will lead to a lot of confusion for people not familiar with the reasons we did it.

I had similar concerns.

So I guess we will have to bite the bullet and revert to having the browserslist in both package.json and .babelrc, and make a note in the docs to make people aware that they have to make changes to browserlist in two places until Babel 7 has landed.

Okay, removing a commit is easy enough. Where do you think I should update the docs?

[LinusBorg]

Okay, removing a commit is easy enough. Where do you think I should update the docs?

@sudo-suhas Let's revert the code first and worry about docs in a second step - we currently don't have a section dealing with the Babel setup at all, so I don't think there's a good place to drop in a quick tipp for the user.

Maybe in the generated project README?

[sudo-suhas]

I already removed the commit which setup .babelrc.js. I'll be able to update the docs tomorrow. If you prefer I do that in a different PR, this can be merged in now.

[LinusBorg]

(close-reopen to trigger CI)