Refactoring, env, closing a bunch of issues by denysdovhan · Pull Request #53 · babel/babel-brunch

denysdovhan · 2017-03-09T20:32:51Z

Aims to fix #52 and #41.

shvaikalesh · 2017-03-10T10:32:27Z

index.js

+
+      try {
+        const babelrc = fs.existsSync(babelrcPath);
+        const packageConfig = JSON.parse(fs.readFileSync(packagePath));


shvaikalesh · 2017-03-10T10:33:06Z

index.js

-    if (!opts.presets) opts.presets = ['latest'];
-    if (opts.presets.length === 0) delete opts.presets;
+
+    // Include 'latest' preset only if:


Comments are too length and extra. Code is pretty self-explanatory.

Maybe better to leave a link to related issues? Just an reminder why this code shouldn't be removed.

Yeah, please do.

shvaikalesh · 2017-03-10T10:33:17Z

index.js

+    //  * there's no .babelrc file in root directory
+    //  * there's no 'babel' field in package.json
+    if (!opts.presets) {
+      const babelrcPath = path.resolve(config.paths.root, '.babelrc');


Extensions? Parent dirs?

Yeah, thought about it.

I don't want to duplicate whole resolution logic from Babel. Maybe we could reuse it somehow?

Not sure, it is not in the public API afaik, but it may be worth exposing.

However, I don't see it blocking from merging this PR. Some time later.

I'm gonna dig a bit in Babel's sources. I'll keep you updated.

shvaikalesh · 2017-03-10T11:05:12Z

One more thing: eslint-config-brunch if you are up to. Thanks.

denysdovhan · 2017-03-10T17:48:53Z

index.js

  return error;
 };

+const lookup = (where, filename) => {


@shvaikalesh here's a little bit complex resolution. What do you think?

loganfsmyth · 2017-03-10T19:03:25Z

Generally I think the Babel team as a whole has mixed feelings about enabling plugins/presets by default like this FYI, but it seems like that has already been released as a feature, right?

Given that, the currently-used approach (in babel-loader and babel-register) to resolving the config is the following:

const OptionManager = require('babel-core').OptionManager;

const opts = new OptionManager().init({
    // whatever options you'd pass to Babel normally.
});

// Check both, not just `plugins`.
const hasConfig = 
  (opts.plugins && opts.plugins.length > 0) ||
  (opts.presets && opts.presets.length > 0);

if (!hasConfig) {

}

It's an ugly API but please do not try to hardcode this. If we fix bugs or add new config methods like the newly-landed for 7.x .babelrc.js, your code will just break again.

denysdovhan · 2017-03-11T10:36:17Z

@shvaikalesh reasonable notice from Babel's Slack:

Actually, this PR could be easily resolved by removing these lines:

-if (!opts.presets) {
-     opts.presets = ['latest'];
-     // ['env', opts.env]
-}

Unfortunately, it would break backward compat.

paulmillr · 2017-03-11T12:07:29Z

I don't understand why "having defaults" is a bad idea. Brunch is all about simplicity and no bullshit. We don't want users to write configs when the program can think for them instead.

denysdovhan · 2017-03-11T13:35:02Z

@paulmillr okay. Anyway, I'm gonna find a better solutions. Will update my PR soon.

@loganfsmyth don't we run the risk using some kind of internal APIs? Probably, you would like to change Babel's internal API and then we will find ourselves in the situation when our users get broken builds.

loganfsmyth · 2017-03-11T18:54:32Z

don't we run the risk using some kind of internal APIs? Probably, you would like to change Babel's internal API and then we will find ourselves in the situation when our users get broken builds.

I'm not sure what you mean? The point of the OptionManager API is to get just the data you're asking about.

shvaikalesh · 2017-03-12T03:17:28Z

Agreed with Paul here. We should have cool defaults, like babel-preset-env. Related: #54

denysdovhan · 2017-03-13T08:54:39Z

I'm not sure what you mean?

@loganfsmyth OptionManager is an internal API, isn't it? That means you wouldn't warn us when you change that API.

loganfsmyth · 2017-03-13T09:02:20Z

It definitely won't break in 6.x and I'm planning to specifically leave a dummy class of export class OptionManager { init(opts){ ...call internal api... } } in 7.x for backward-compatibility because we want the transition from 6.x to 7.x to be as seamless as possible for people.

It's used by babel-register and babel-loader, which are both independent of babel-core, so any change to it at this point would also break those, which is a no-go for us.

shvaikalesh · 2017-03-21T17:14:13Z

test.js

-    });
+    return plugin.compile({data: content, path: 'file.js'})
+      .then(result => result.data.should.contain(expected))
+      .catch(should.not.throw);


shvaikalesh · 2017-03-21T17:14:22Z

test.js

-    }, error => assert(!error));
+    return plugin.compile({data: content, path: 'file.js'})
+      .then(result => result.data.should.contain(expected))
+      .catch(should.not.throw);


shvaikalesh · 2017-03-21T17:14:27Z

test.js

-    }, error => assert(!error));
+    return plugin.compile({data: content, path: 'file.js'})
+      .then(result => result.data.should.contain(expected))
+      .catch(should.not.throw);


shvaikalesh · 2017-03-21T17:14:38Z

test.js

+
+    return plugin.compile({data: content, path: 'file.js'})
+      .then(result => result.data.should.contain(expected))
+      .catch(should.not.throw);


shvaikalesh · 2017-03-21T17:14:50Z

test.js

+
+    return plugin.compile({data: content, path: 'file.js'})
+      .then(result => result.data.should.contain(expected))
+      .catch(should.not.throw);


shvaikalesh · 2017-03-21T17:15:10Z

test.js

+    it('should properly link to source file in source maps', () =>
+      sourceMapPlugin.compile({data: content, path})
+        .then(result => {
+          JSON.parse(result.map).should.not.throw;


shvaikalesh · 2017-03-21T17:15:18Z

test.js

+    );
+
+    it('should properly link to source file in source maps', () =>
+      sourceMapPlugin.compile({data: content, path})


shvaikalesh · 2017-03-21T17:15:23Z

test.js

+          JSON.parse(result.map).should.not.throw;
+          JSON.parse(result.map).sources.should.contain(path);
+        })
+        .catch(should.not.throw)


shvaikalesh · 2017-03-21T17:15:29Z

test.js

-    }, error => assert(!error));
+    return plugin.compile({data: content, path: 'file.js'})
+      .then(result => JSON.parse(result.map).should.not.throw)
+      .catch(should.not.throw);


shvaikalesh · 2017-03-21T17:15:43Z

test.js

-      done();
-    }, error => assert(!error));
+    return plugin.compile({data: content, path: 'file.js'})
+      .then(result => JSON.parse(result.map).should.not.throw)


should.not.throw is extra

shvaikalesh · 2017-03-21T17:16:03Z

test.js

-    }, error => assert(!error));
+    return plugin.compile({data: content, path: 'vendor/file.js'})
+      .then(result => result.data.should.be.equal(content))
+      .catch(should.not.throw);


shvaikalesh · 2017-03-21T20:47:17Z

test.js

-  it('should be an object', function() {
-    assert(plugin);
-  });
+  it('should have #compile method', () =>


why return Assertion to mocha?

Does Mocha care about what is returned?

Anyway, yeah, that's not a good practice. Gonna add curly brackets around it.

denysdovhan · 2017-03-26T15:12:44Z

Now babel-brunch uses env, detects UglifyJS and sets corresponding target if it's needed. At this point, it should be enough to fix #52, #54 and #41.

@shvaikalesh @paulmillr what do you think about targets options for babel-brunch proposed by @yavorsky? (#53 (comment))

shvaikalesh · 2017-03-26T16:52:17Z

index.js

+  try {
+    const pkgPath = path.resolve(pkgFile);
+    const isProduction = process.env.NODE_ENV === 'production';
+    const isUglify = 'uglify-js-brunch' in require(pkgPath).devDependencies;


require('uglify-js-brunch')

denysdovhan · 2017-03-28T08:00:32Z

Done, I think. Can be merged.

shvaikalesh · 2017-03-28T10:10:56Z

index.js

+    if (!this.options.presets) {
+      const babelConfig = new babel.OptionManager().init(this.options);
+      const hasConfig =
+        babelConfig.presets && babelConfig.presets.length > 0 ||


JS is not Ruby. 0 is falsy.

JS is not Ruby. 0 is falsy.

Correct me if I'm wrong, and it's not immediately clear, but this should work because @denysdovhan is checking length > 0.

I suggest wrapping the comparison check in parens so it is more obvious that you're using a comparison operator.

const hasConfig = babelConfig.presets && (babelConfig.presets.length > 0) || babelConfig.plugins && (babelConfig.plugins.length > 0);

@millerized @shvaikalesh is right. Doesn't matter. This should work too:

const hasConfig = babelConfig.presets && babelConfig.presets.length || babelConfig.plugins && babelConfig.plugins.length;

millerized · 2017-03-29T02:40:41Z

Hi @denysdovhan. I would offer to help contribute and get this past the finish line, but it looks like you are almost there.

I am looking forward to using brunch with env support – great work! 🍻

denysdovhan added 2 commits March 9, 2017 22:16

Remove presets and plugin mapping. Babel does this just fine.

4d50d45

Less brackets

f2a6bf3

denysdovhan changed the title ~~Resolve presets~~ WIP: Resolve presets Mar 9, 2017

denysdovhan added 2 commits March 9, 2017 22:40

Have no idea what is this for

4cbfb1a

Add explanation comment

3abb88e

shvaikalesh reviewed Mar 10, 2017

View reviewed changes

Require instead of plain reading and parsing

ffa3170

This was referenced Mar 10, 2017

transform-class-properties in .babelrc results in error #52

Closed

Use options defined in the .babelrc file #41

Closed

denysdovhan changed the title ~~WIP: Resolve presets~~ Resolve presets Mar 10, 2017

shvaikalesh reviewed Mar 10, 2017

View reviewed changes

denysdovhan added 6 commits March 10, 2017 14:21

Add ESLint and eslint-config-brunch

83b988a

Fix linting errors in index.js

189c1c5

Lint all files

2e438ab

Fix linting errors in test.js

69c85da

Fix for old NPMs

577bd5d

Add config lookup function

fdeca5f

denysdovhan commented Mar 10, 2017

View reviewed changes

denysdovhan changed the title ~~Resolve presets~~ WIP: Resolve presets Mar 11, 2017

Fix README.md

db91059

shvaikalesh reviewed Mar 21, 2017

View reviewed changes

Remove extras from tests

ff43af8

shvaikalesh reviewed Mar 21, 2017

View reviewed changes

loganfsmyth mentioned this pull request Mar 24, 2017

Understanding usecases of manual .babelrc lookups and OptionManager use babel/babel#5541

Closed

Detect UglifyJS and set corresponding target

fb76f68

Don't return Assertion to Mocha

d267853

denysdovhan mentioned this pull request Mar 26, 2017

[BREAKING] Use babel-preset-env instead of babel-preset-latest #54

Closed

shvaikalesh reviewed Mar 26, 2017

View reviewed changes

denysdovhan added 2 commits March 27, 2017 23:42

Better: useUglify -> targetUglify

821b3e8

Update and clean-up README.md

6f796ac

denysdovhan changed the title ~~WIP: Refactoring, env, closing a bunch of issues~~ Refactoring, env, closing a bunch of issues Mar 27, 2017

shvaikalesh reviewed Mar 28, 2017

View reviewed changes

denysdovhan added 3 commits March 29, 2017 19:20

Remove useless checking

f685b11

Explicitly return false if it's not production, but UglifyJS present

d54b653

Update Travis configuration

868409b

paulmillr merged commit df0e92b into babel:master Mar 29, 2017

webdif mentioned this pull request Aug 11, 2017

.babelrc is ignored #61

Closed

Uh oh!

Conversation

denysdovhan commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shvaikalesh commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loganfsmyth commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

denysdovhan commented Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulmillr commented Mar 11, 2017

Uh oh!

denysdovhan commented Mar 11, 2017

Uh oh!

loganfsmyth commented Mar 11, 2017

Uh oh!

shvaikalesh commented Mar 12, 2017

Uh oh!

denysdovhan commented Mar 13, 2017

Uh oh!

loganfsmyth commented Mar 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shvaikalesh Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denysdovhan commented Mar 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denysdovhan commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loganfsmyth commented Mar 10, 2017 •

edited

Loading

denysdovhan commented Mar 11, 2017 •

edited

Loading

shvaikalesh Mar 21, 2017 •

edited

Loading

denysdovhan Mar 29, 2017 •

edited

Loading