Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop reading .babelrc files. #398

Merged
merged 1 commit into from
Jan 3, 2016

Conversation

jamestalmage
Copy link
Contributor

If a .babelrc file is present, it is being read, and the settings merged with the ones we specify for tests. We don't want that.

Reference:
http://babeljs.io/docs/usage/options/

@jamestalmage
Copy link
Contributor Author

I still need to write a test for this. I can get to it later today.

@sindresorhus
Copy link
Member

@jamestalmage It's already tested in Babel I assume, but sure, if you want.

@JaKXz
Copy link

JaKXz commented Jan 1, 2016

FWIW, using this branch fixed the issue I was having. Thanks again @jamestalmage.

@jamestalmage
Copy link
Contributor Author

It's already tested in Babel I assume

Yeah, but I could have sworn I already addressed this once before, adding a regression test ensures we don't accidentally drop it.

Test added.

@ariporad
Copy link
Contributor

ariporad commented Jan 2, 2016

Wait, @jamestalmage, @sindresorhus: Why wouldn't we want this? (Sorry if I missed some earlier conversation).

@jamestalmage
Copy link
Contributor Author

Why wouldn't we want this?

Because it prevents them from using a .babelrc file for instrumenting their production code with different settings from the tests. We don't currently support people modifying the transforms applied to their tests (and if we ever do, it almost certainly won't be via a .babelrc file).

@ariporad
Copy link
Contributor

ariporad commented Jan 2, 2016

@jamestalmage: Ok, but as a user of AVA, having AVA transpile my tests with babel, yet not follow my .babelrc, while recommending the babel require hook for test helper files (which does follow my .babelrc) is incredibly confusing. Like really confusing.

@jamestalmage
Copy link
Contributor Author

While recommending the babel require hook for test helper files.

We will support compiling helpers soon (#177 was an early attempt). Once #390 is merged, I will work on that.

@sindresorhus
Copy link
Member

@jamestalmage Looks good to me. Just merge when you've fixed the merge conflict ;)

@ariporad
Copy link
Contributor

ariporad commented Jan 3, 2016

@jamestalmage, @sindresorhus: I would like to say that I really think that if we're going to block .babelrc (I don't think we should). Then we should prioritize allowing additional babel config, because being forced to use diffrent babel settings for tests and production code is really bad.

@jamestalmage
Copy link
Contributor Author

We are already doing that. Without this change, our babel settings from caching-precompiler/test-worker are being merged with their .babelrc, and the user has no recourse to change that.

If users really want power-assert, and stage-2 constructs for their production code then they should list that explicitly in their .babelrc.

because being forced to use diffrent babel settings for tests and production code is really bad.

I don't think that's true at all, in fact I think the opposite. If your production code requires Babel, then you need to include a build step as part of npm publish to transpile your code to ES5 so it is easily consumed on npm. A build step necessitates a complete.babelrc for your production code, not one that relies on some plugins your test runner automagically merges (Especially since AVA should be a dev-dependency which would not be deployed with your production code). A complete .babelrc also clearly documents what code constructs can be used in production.

@ariporad
Copy link
Contributor

ariporad commented Jan 3, 2016

@jamestalmage: That's not what I meant. Here's my situation: I use the babel-require hook, with babel-preset-es2015, and babel-preset-stage-0 in my .babelrc. I'd like for AVA to use that. Otherwise there's stuff I can do in my code but not in my tests. On top of that, the babel-runtime thing means that I can't use bluebird, which is extremely annoying. So I think that the best course of action would be to make AVA smart enough to detect a competing .babelrc, and if it exists, use that (+ power-assert), instead of it's own config.

@jamestalmage
Copy link
Contributor Author

On top of that, the babel-runtime thing means that I can't use bluebird

Why not?

detect a competing .babelrc, and if it exists, use that (+ power-assert), instead of it's own config...

I don't like that idea. I am certainly open to the idea of allowing users to customize the test transpiler as they see fit. (If you hunt back into the early 100's you'll find half a dozen PR's where I explore the idea). Doing it automatically isn't a good idea, users should have to be explicit about it.

jamestalmage added a commit that referenced this pull request Jan 3, 2016
@jamestalmage jamestalmage merged commit 87a3430 into avajs:master Jan 3, 2016
@jamestalmage jamestalmage deleted the dont-read-babelrc branch January 3, 2016 03:09
@ariporad
Copy link
Contributor

ariporad commented Jan 3, 2016

@jamestalmage: your can't use bluebird because it overrides 'Promise' anywhere it finds it, regardless of scope.

Why would it ever be a bad thing for babel to read babel configuration?

What I've personally done for a project that i started yesterday was to transpile it with babel ahead of time.

So while I think that es2015 support is a cool idea, I think that has having a '--no-babel' flag for more complex projects. might be a good idea. (And, you know, being able to babel config would be great too).

Thoughts?

@jamestalmage
Copy link
Contributor Author

it overrides 'Promise' anywhere it finds it, regardless of scope.

Well that's annoying.

As for the rest of it:

I am certainly open to the idea of allowing users to customize the test transpiler as they see fit ... [but] users should have to be explicit about it.

@jamestalmage
Copy link
Contributor Author

it overrides 'Promise' anywhere it finds it, regardless of scope.

Well that's annoying.

It's also not true:

// foo.js
import Promise from 'bluebird'
import test from '../../'

Promise.longStackTraces();

test(async t => {
    await Promise.resolve('foo');
});
// transpiled foo.js
'use strict';

var _regenerator = require('babel-runtime/regenerator');

var _regenerator2 = _interopRequireDefault(_regenerator);

var _asyncToGenerator2 = require('babel-runtime/helpers/asyncToGenerator');

var _asyncToGenerator3 = _interopRequireDefault(_asyncToGenerator2);

var _bluebird = require('bluebird');

var _bluebird2 = _interopRequireDefault(_bluebird);

var _ = require('../../');

var _2 = _interopRequireDefault(_);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

_bluebird2.default.longStackTraces();

(0, _2.default)((function () {
    var _this = this;

    var ref = (0, _asyncToGenerator3.default)(_regenerator2.default.mark(function _callee(t) {
        return _regenerator2.default.wrap(function _callee$(_context) {
            while (1) {
                switch (_context.prev = _context.next) {
                    case 0:
                        _context.next = 2;
                        return _bluebird2.default.resolve('foo');

                    case 2:
                    case 'end':
                        return _context.stop();
                }
            }
        }, _callee, _this);
    }));
    return function (_x) {
        return ref.apply(this, arguments);
    };
})());

@ariporad
Copy link
Contributor

ariporad commented Jan 3, 2016

@jamestalmage: Weird... It didn't work for me, and according to the maintainers, it shouldn't. I still disagree that AVA shouldn't read the config automatically, but it's not that big of a deal if we document it.

@sindresorhus: Thoughts on a --no-babel option as a hold over till we allow customizing babel config?

@sindresorhus
Copy link
Member

Thoughts on a --no-babel option

For what purpose exactly?

@ariporad
Copy link
Contributor

ariporad commented Jan 3, 2016

@sindresorhus: Until we can modify babel config, I pre-compile my code with babel. Running the transpiled code through babel again doesn't make any sense (This can come with a warning like If you use --no-babel, you must use the power assert babel plugin!).

@jamestalmage
Copy link
Contributor Author

This can come with a warning.

Or not. We already handle assertions that are not enhanced by the plugin (throws, doesNotThrow).

Personally, I would rather just wait until we figure out configurable transforms instead of adding a feature we intend to replace.

@ariporad - Would the ability to include stage-1 and/or stage-0 satisfy your needs? I am thinking something like a --stage=0 flag.

@ariporad
Copy link
Contributor

ariporad commented Jan 3, 2016

@jamestalmage: Not really. I also use babel-preset-features, although I guess loosing that wouldn't be the end of the world. But really I think the solution is to allow custom babel config.

@billyjanitsch
Copy link
Contributor

@jamestalmage I agree with @ariporad -- it's unintuitive that if my lib code is using babel-preset-stage-[<2] and/or other custom plugins (e.g. I use one to extend import syntax), then I can't use these features in my test code.

Personally, I think the --require flag should disable the Babel hook in favor of the user-provided register hook, but something like a --no-babel option would work too.

More generally, AVA works well for the (increasingly less common) case of packages written in ES5 with no transpilation step (since you get to write your tests using modern syntax "for free"). However, I'd argue that it drops the ball for the (increasingly more common) case where library code is transpiled anyway: now, despite you having set up tooling to use the syntax you want in lib code, you have to write your tests in a predetermined (and typically strictly smaller) subset of new syntax.

@jamestalmage
Copy link
Contributor Author

I've never said we won't allow configuration of how your tests get transpiled, that's a feature I think is worthwhile. Just that it's not going to automatically happen because you have a .babelrc file present.

I'm not sure what's "most common"/"typical", though I am sure ES2015 use is on the rise. I do find it hard to believe that stage-1/0 constructs are in high demand, especially for use in production code. That seems like a risky plan.

However we solve configurable transpiling, I want it to be able to participate in the caching-main thread-precompiler stuff we just added that has given us a huge performance boosts.

@ariporad
Copy link
Contributor

ariporad commented Jan 7, 2016

@jamestalmage: Think about it like this: AVA says that it invokes babel for you. If you've explicitly said that you want babel to do X, it would be confusing when AVA's babel doesn't.

Since this seems to be a matter of opinion, why don't we just do a poll?

@JaKXz
Copy link

JaKXz commented Jan 7, 2016

@ariporad @billyjanitsch I disagree that it would be confusing, because of the docs outlining AVA's approach to ES2015 that say:

AVA comes with builtin support for ES2015 through Babel 6. Just write your tests in ES2015. No extra setup needed. You can use any Babel version in your project. We use our own bundled Babel with the es2015 and stage-2 presets.

Of course I'm a bit biased towards this because this PR solved the issue I was having with conflicting .babelrc settings breaking the test execution. Even so, I think it's better that AVA's set up is completely independent of my project because it's one less thing to worry about (say I'm porting older Mocha tests from a Babel 5.x project one day, and starting a brand new project the next - the syntax for my tests hasn't changed so I can write my tests first for both projects and not have to rethink structure/syntax/language features/etc). I also think it's better to test production code with esnext features that are more mature & less prone to inconsistent behaviour, for sanity's sake.

A healthy compromise IMHO would be an --ignore-my-babelrc option that defaulted to true and could be configured via the command line or package.json. Something along those lines (with perhaps a snappier name) I would love to see for the day when I would like to use more advanced Babel settings in my tests for whatever reason.

@ariporad
Copy link
Contributor

ariporad commented Jan 7, 2016

@sindresorhus: what are your thoughts on having AVA's babel automatically ignore the explicit babel configuration that users have set. (In my mind, this seems absolutely crazy and the wrong thing to do).

And since this seems to be a debated topic, would everyone be open to a poll of AVA users on what they'd prefer?

@MoOx
Copy link

MoOx commented Jan 17, 2016

This is a really weird breaking change. I am using AVA to test some react (jsx yeah) and now I just cannot (=> I cannot upgrade to 0.10 and I am stuck with 0.9 until there is a solution).

Are you aware that babel have an option to specify config depending on the NODE_ENV variable?
Because it seems that the thing that should be used here: http://babeljs.io/docs/usage/babelrc/#env-option (make sense in the use case of @JaKXz).

AVA should use babelrc by default, otherwise UX is like superweird.

@jamestalmage
Copy link
Contributor Author

@MoOx - as stated elsewhere. The problem is that without this, the configs end up getting merged with the one AVA automatically creates(which is not always what you want). Specifically, I would generally prefer only to ship production code with fully ratified language constructs (i.e. only the es2015 preset), whereas I'm perfectly comfortable using "beta" language features in my tests.

We are working on a --no-babel option which will allow you to disable AVA's babel processing entirely, and let you do your own thing.

Are you aware that babel have an option to specify config depending on the NODE_ENV variable

I wasn't until just a few hours ago, and my thought was similar to yours. My question now is: Is there a way to specify an environment on a per-file basis via the API?

@MoOx
Copy link

MoOx commented Jan 17, 2016

the configs end up getting merged with the one AVA automatically creates(which is not always what you want).

Is there something uncommon that AVA need?

Is there a way to specify an environment on a per-file basis via the API?

No idea. But maybe process.env.BABEL_ENV/NODE_ENV can be adjusted during the runtime (because babel read from that)?

@jamestalmage
Copy link
Contributor Author

AVA automatically includes stage-2, which is pretty beta stuff. Also, AVA adds a transform required for power-assert which you definitely don't want instrumenting your production code.

@jamestalmage
Copy link
Contributor Author

Let's continue further discussion in #448.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants