Skip to content

Automatically apply stage-4 preset even when Babel options are customized #1488

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

Closed
lukechilds opened this issue Aug 12, 2017 · 17 comments
Closed
Labels

Comments

@lukechilds
Copy link
Contributor

The ava.babel property of package.json overwrites the default babel config. This means if you want to add support to for JSX for example in your tests, you can't just do:

npm install --save-dev babel-plugin-transform-react-jsx
"ava": {
  "babel": {
    "plugins": ["transform-react-jsx"]
  }
}

Because it overwrites the default babel presets with undefined so you get loads of syntax errors. You can get it working by manually also specifying the default @ava/stage-4 preset:

"ava": {
  "babel": {
    "plugins": ["transform-react-jsx"],
    "presets": ["@ava/stage-4"]
  }
}

However, this is less than optimal because if AVA upgrades to a new default in the future I'll get a "module not found" error. I could manually add @ava/stage-4 as a dev dependency to my project to solve this but then that means I'll be out of sync with AVA's default presets if AVA updates.

I think the best solution would be for AVA to merge custom options into it's defaults so you can literally extend the settings.

@lukechilds
Copy link
Contributor Author

Actually I see why this may be a problem. Someone may want to completely overwrite the default @ava/stage-4 with their own presets. So maybe merging isn't the best solution.

My original issues are still relevant though, I think it would be better if there was a way to extend the default AVA babel config without having to copy the defaults.

@novemberborn
Copy link
Member

This is implemented in these lines: https://github.com/avajs/ava/blob/b6eef5ac30e7839371a420f17060f62f971717aa/lib/babel-config.js#L100:L127

We have a baseConfig, which contains the @ava/transform-test-files preset. If package.json#ava.babel is missing or "default" we also add @ava/stage-4 to the baseConfig's presets.

If package.json#ava.babel is "inherit" we make baseConfig extend from {babelrc: true}. Otherwise it should be an object, and we make it extend from that object. This follows Babel's handling of the extends property.

Typically the problem users run into is that module syntax is no longer transpiled. As you say it'd be great if we would still include @ava/stage-4 but this isn't always the right answer.

Given that we already resolve the config ourselves, we could default to adding the @ava/stage-4 preset when package.json#ava.babel is set to an object, and have an option to disable this? "inherit" would always leave the preset disabled. I'm not quite sure how to name that option though.

Another approach may be to explicitly re-export @ava/stage-4 under ava/stage-4, so that you don't feel like you have to install it as a devDependency.

With either approach, naming is the problem. We always want to apply the @ava/transform-test-files preset, so just saying "avaPresets" wouldn't be accurate.

@lukechilds
Copy link
Contributor Author

What about an extend option?

Something like:

"ava": {
  "babel": {
    "extend": {
      "plugins": ["transform-react-jsx"]
    }
  }
}

The above config makes it clear I always want the default AVA babel config, but just extend it with "transform-react-jsx".

I'm not keen on that exact example as it's not clear how it should be handled if both ava.babel.extend and ava.babel.preset were set but I think that's kinda in the right direction.

What do you think?

Another approach may be to explicitly re-export @ava/stage-4 under ava/stage-4, so that you don't feel like you have to install it as a devDependency.

The issue wasn't so much installing the dev dependency, it was hardcoding stage-4. For example even if you export through AVA and I just set the preset to ava/stage-4, if in the future AVA decides to use a different default preset I'm gonna be stuck on stage-4 or get an error if that file gets deleted.

@novemberborn
Copy link
Member

The proposal in https://github.com/avajs/ava/blob/master/docs/specs/001%20-%20Improving%20language%20support.md#babel-projects is to always disable the default AVA behavior whenever anything is customized. It suggests an ava preset value. Perhaps that doesn't have to be an actual preset — we could rewrite it on the fly to whatever the current presets are.

Applied to the current situation you'd get:

"ava": {
  "babel": {
    "plugins": ["transform-react-jsx"],
    "presets": ["ava"]
  }
}

The problem though is that you can't use ava as a preset in regular .babelrc files. So perhaps using the preset syntax isn't the right approach.

This is just odd, though:

"ava": {
  "babel": {
    "ava": true,
    "plugins": ["transform-react-jsx"]
  }
}

But I think it's on the right track. Just need to come up with a better property name than ava.

@lukechilds
Copy link
Contributor Author

"ava": {
  "babel": {
    "plugins": ["transform-react-jsx"],
    "presets": ["ava"]
  }
}

Expresses the intent perfectly IMO.

Is it an issue that it can't be used in .babelrc? It's only meant to change the babel config for tests not for the project.

If so, what about:

"ava": {
  "babel": {
    "extendsDefault": true,
    "plugins": ["transform-react-jsx"]
  }
}

@novemberborn
Copy link
Member

novemberborn commented Aug 12, 2017

Is it an issue that it can't be used in .babelrc? It's only meant to change the babel config for tests not for the project.

I think it's misleading. Users may be tempted to move it into an external file.

Let's assume extendsDefault is the default. Would setting it to false also disable @ava/transform-test-files? Perhaps we can pick a negated name, e.g. disableAvaPresets.

If we go this route, perhaps the "inherit" setting should be equivalent to {"disableAvaPresets": true, "babelrc": true}?

@lukechilds
Copy link
Contributor Author

I think it's misleading. Users may be tempted to move it into an external file.

👍

Let's assume extendsDefault is the default. Would setting it to false also disable @ava/transform-test-files? Perhaps we can pick a negated name, e.g. disableAvaPresets.

Extending by default and having a disableAvaPresets sounds like the best solution so far. I just assumed it would extend by default and was pretty stumped why everything broke until I checked the docs. This would be a breaking change though right? Because after this change some people will get @ava/stage-4 who didn't previously.

If we go this route, perhaps the "inherit" setting should be equivalent to {"disableAvaPresets": true, "babelrc": true}?

That makes sense to me.

@novemberborn novemberborn changed the title Merge package.json ava.babel settings Automatically apply stage-4 preset even when Babel options are customized Aug 13, 2017
@novemberborn
Copy link
Member

Extending by default and having a disableAvaPresets sounds like the best solution so far.

👍

(Note that semantically, AVA's default extends the user config, not the other way around.)

This would be a breaking change though right?

Yes.

If we go this route, perhaps the "inherit" setting should be equivalent to {"disableAvaPresets": true, "babelrc": true}?

That makes sense to me.

👍

This may be a good time to add support for "babel": false, too.


Would you like to take this on @lukechilds?

@lukechilds
Copy link
Contributor Author

I'm happy to take it on, however I've spent quite a bit of time on OSS recently and ideally need to focus on paid work for a bit so can't guarantee it'll be done soon.

@novemberborn
Copy link
Member

No worries, I know the feeling 😄

If anybody else wants to pick this up in the meantime, please drop a line!

@vadimdemedes
Copy link
Contributor

@lukechilds Do you think this would make the extending easier? #1225

ava/babel (babel.js in root) would always export the current Babel setup, therefore situations like the following shouldn't occur:

if in the future AVA decides to use a different default preset I'm gonna be stuck on stage-4 or get an error if that file gets deleted.

@lukechilds
Copy link
Contributor Author

@vadimdemedes You mean so we could have this: #1488 (comment)?

If the .babelrc issue that @novemberborn raised isn't a concern then yeah, I think that's a great solution.

@vadimdemedes
Copy link
Contributor

Yes, only with that example:

"ava": {
  "babel": {
    "plugins": ["transform-react-jsx"],
    "presets": ["ava"]
  }
}

we'd need to have babel-preset-ava pkg, which is also a possibility, but not that different from what we have now. Only a different, even though nicer, name. ava/babel seems to follow the established convention of bundling pkg-related babel presets/plugins at pkg/babel.

@lukechilds
Copy link
Contributor Author

"ava": {
  "babel": {
    "plugins": ["transform-react-jsx"],
    "presets": ["ava/babel"]
  }
}

is equally beautiful to me ❤️

@vadimdemedes
Copy link
Contributor

vadimdemedes commented Aug 13, 2017

As for @novemberborn's concern about using AVA's presets in .babelrc, it would still be possible with ava/babel, if anyone wants to do that (I never do). At least one popular pkg example which uses this "convention" is glamor.

@novemberborn
Copy link
Member

That would work (and we could handle the powerAssert option to @ava/transform-test-files). But it won't be the default, and I suspect that is what's tripping most people up.

@novemberborn novemberborn added this to the 1.0 milestone Aug 28, 2017
novemberborn added a commit that referenced this issue Jan 26, 2018
Always extend the project's Babel configuration, if any.

Always apply the `@ava/stage-4` preset. Fixes #1488. Users can disable
this preset by adding it to the AVA's Babel configuration with the
`false` option:

```
"ava": {
	"babel": {
		"presets": [
			["ava/stage-4", false]
		]
	}
}

And yes, `ava/stage-4` so now an alias for the preset, which means it
can be used even if the preset is not installed as a top-level
dependency. Fixes #1225.

`@ava/transform-test-files` is applied as part of the test compilation,
but cannot be overridden by users like `@ava/stage-4` can.
@novemberborn
Copy link
Member

#1608 now always applies @ava/stage-4, but it can be disabled whilst still allowing other Babel options to be used for test file compilation.

novemberborn added a commit that referenced this issue Jan 27, 2018
Always extend the project's Babel configuration, if any.

Always apply the `@ava/stage-4` preset. Fixes #1488. Users can disable
this preset by adding it to the AVA's Babel configuration with the
`false` option:

```
"ava": {
	"babel": {
		"presets": [
			["ava/stage-4", false]
		]
	}
}

And yes, `ava/stage-4` so now an alias for the preset, which means it
can be used even if the preset is not installed as a top-level
dependency. Fixes #1225.

`@ava/transform-test-files` is applied as part of the test compilation,
but cannot be overridden by users like `@ava/stage-4` can.
novemberborn added a commit that referenced this issue Jan 27, 2018
Fixes #1598. Switches AVA's Babel implementation to use Babel 7. This applies to test and helper file compilation.

Adds a `compileEnhancements` option which can be set to `false` to disable Power Assert and our `t.throws()` helper.

Changes the Babel configuration. If you had this before:

```json
"ava": {
  "babel": {
    "plugins": []
  }
}
```

You'll now need:

```json
"ava": {
  "babel": {
    "testOptions": {
      "plugins": []
    }
  }
}
```

`ava.babel.testOptions.babelrc` now defaults to `true`. You can disable our stage-4 preset by adding `["ava/stage-4", false]` to `ava.babel.testOptions.presets`. Set `ava.babel` to `false` to disable AVA's test file compilation, **whilst still compiling enhancements**. If `compileEnhancements` is *also* set to `false` then Babel is skipped completely.

Fixes #1225, #1488 and #1556.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants