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 transpiling ESM out of the box? #1908

Closed
novemberborn opened this issue Aug 15, 2018 · 15 comments
Closed

Stop transpiling ESM out of the box? #1908

novemberborn opened this issue Aug 15, 2018 · 15 comments
Labels
breaking requires a SemVer major release future we'll get back to this eventually question

Comments

@novemberborn
Copy link
Member

Currently we transpile ESM syntax in test files. This has frequently caused confusion when users also rely on that syntax in their source files, but are unclear how to configure AVA to transpile sources. #1905 (comment) being the most recent example.

Further, we only transpile syntax, without implementing the specced behavior. Now that esm is available that is probably the better choice for most users. However I don't think it's something we should default to.

I wonder if it would be clearer to not transpile ESM syntax, as well as remove it from our examples. We can then more clearly communicate the available options for supporting ESM:

  • Configure AVA's Babel pipeline to convert to CJS, which introduces users to the Babel pipeline and lets us more clearly signal how to configure source compilation
  • Configure esm

We could consider exposing esm as ava/esm, since we already have a dependency on it. Similarly we could implement the ESM-to-CJS compilation through our ava/stage-4 plugin. I'm leaning towards making people explicitly install esm or the @babel/plugin-transform-modules-commonjs packages.

It'll also be easier to support Node.js experimental ESM support, especially now that we support custom file extensions.

What do folks think?

@sholladay
Copy link

I wouldn't mind too much either way, but I will say that for me personally, AVA was a nice, practical introduction to ES6 and ESM. There are a lot of blog posts talking about ESM, but it barely has any usage yet outside of projects with a Babel build step. With AVA, it feels native even though it's not, since it's zero-config for test files, and I like that. It feels like the future (and it is).

I've always found it pretty easy to test CJS modules with AVA. I think testing Babel projects is a bit harder, but that seems expected to me.

As for how to reduce the mental burden on users, I have a few ideas:

  1. I think this is a great case for a flow chart. AVA basically has two different setup flows, depending on whether your project needs Babel or not. That lends itself well to a visual explanation in a diagram and I think it's a good way to express that you should make a choice from the beginning.
  2. create-ava should detect my usage of Babel and configure AVA based on that. For example, put @babel/register in ava.require in package.json for me. And output some friendly messages related to Babel setup. Feature create-ava more prominently in the README, recipes, and blog posts. Get people to use it and lean on its automation.
  3. Official Yeoman generators for both Babel and non-Babel "AVA Hello World" setups would be super useful to people.

@sindresorhus
Copy link
Member

sindresorhus commented Aug 19, 2018

I don't think it's worth breaking pretty much everyone's tests just because some users don't understand how it works. The solution here is better docs.

I do agree the esm module is a better than transpiling the syntax with Babel, but I don't think it's worth this big of a breakage.

@sindresorhus
Copy link
Member

Configure AVA's Babel pipeline to convert to CJS, which introduces users to the Babel pipeline and lets us more clearly signal how to configure source compilation

I was hoping AVA would rather move to a model where it either transpiles everything (or the specified files to be transpiled) or nothing. That way there's no confusion.

@novemberborn
Copy link
Member Author

Thanks @sholladay. I agree with all your points.

For Babel projects, I agree that the biggest hurdle is that AVA doesn't compile sources. Especially now that new language features are adopted more quickly this leads to projects that may only be compiled for distribution, either using Babel or webpack. Users get syntax errors when they import non-compiled source files.

For projects using webpack, the extra difficulty is that, if they're also compiled using Babel, that Babel configuration typically does not compile ESM, since that's left to webpack. AVA's test-file only support for ESM can be hugely misleading.

For projects using esm, there's the counter-intuitive step of having to disable AVA's ESM compilation.


The solution here is better docs.

We could revisit our recipes with these project types in mind. We could also detect these syntax errors and point users to the relevant documentation.

I was hoping AVA would rather move to a model where it either transpiles everything (or the specified files to be transpiled) or nothing. That way there's no confusion.

Note though that if user compiles ESM syntax through a different tool (webpack, or esm) AVA won't pick up on this automatically.

I do agree the esm is a better than transpiling the syntax with Babel, but I don't think it's worth this big of a breakage.

Perhaps we need to make it easier to opt-out of AVA's default ESM handling. Right now it involves specifying our ava/stage-4 preset and disabling module compilation. Perhaps it should be a flag, e.g. package.json#ava.babel.compileEsm: Boolean. If we do that, perhaps we could default to compiling ESM even for sources.

@sindresorhus
Copy link
Member

I could be 👍 for this change if we made the breakage worth it:

  • Disable transpilation by default unless there's a babel config.
    I want to remove the performance hit of Babel for users that don't use it and improve stack traces.
  • Remove Babel as a direct dependency.
    We would use the Babel version used by the project if it's in the range supported by AVA.
  • Don't transpile ESM by default when Babel is used.
  • When Babel is used, transpile everything by default, not just tests.

Maybe this needs to wait for AVA v2, but I would still like to do all of this in one go to reduce churn for users.

@novemberborn
Copy link
Member Author

Yes that makes sense. I'd like something similar for TypeScript.

I was thinking to have @ava/babel and @ava/typescript packages, which are required as soon as you enter configuration for babel and typescript. They themselves could have peer dependencies on AVA and Babel / TypeScript respectively.

Agreed that this is for after 1.0.

@novemberborn novemberborn added the future we'll get back to this eventually label Aug 19, 2018
@chrisdothtml
Copy link

I've been trying out ava for the first time today, and can confirm that I totally expected it to transpile my imported files in addition to my test files. I'd be fine with it either transpiling all by default, or none by default (with a single option to enable transpilation of all) as long as it's consistent.

In the interim, though, this worked perfectly fine:

package.json

"ava": {
  "require": "esm"
}

It might be a good idea to explicitly point out in the readme under Transpiling imported modules that this is the simplest solution (until a change is made for v2). Until I found this issue, it wasn't very clear what I needed to do to fix the problem

@revelt
Copy link

revelt commented Sep 1, 2018

To add my 2p, I never expected ava to transpile my imported files in addition to my test files and documentation told that somewhere. @chrisdothtml you're a rare case :)

Personally I found the ava's recipe about bypassing the ESM transpiling a little outdated (talking about Node 8) and a bit hit-and-miss in general. Things are explained but partially. However, I tried out all setups until one worked and settled with that. There's no nyc mentioned there!

All my libraries' sources are in ES6 and I use Rollup to transpile to ESM+CJS+UMD and the main reason behind pointing ava towards ESM build rather than CJS is coverage.

If transpiled CJS files are used to calculate coverage, all functions that Babel adds are left out and the scores suffer (not to mention CJS code lines differ in Coveralls.io from ESM source, for example).

It's ideal when source is ESM and ava runs on ESM build (no transpiling) and coverage is calculated on ESM build.

Sadly, since Babel 7 the usual nyc --require esm --reporter=html --reporter=text ava" does not work and I had to temporarily remove the coverage completely from all my libraries until I find the way and/or dependencies catch up with Babel 7.

Looking at higher level, Rollup does the transpiling and it references the Babel config in library's root. I've got @babel/core as dev-dependency. But then, nyc pipes ava through esm — the whole existence of esm seems questionable to me — in theory, why can't all the parts of the code use the same @babel/core from node_modules and use @babel/preset-env referenced in library's root? Why do we even need esm? But that's more stones to nyc rather than ava's yard. You guys are doing great and I look forward to proper, semantic 1.0.0 release.

@mellster2012
Copy link

Agreed. Really like ava, but when trying to bypass transpiling (node.js v8+) none of the recipes and answers really worked here, only a "mix-and-match" combo. In the (eventually) working solution test and source files (using es6 import/export) have to have the .mjs extension, esm must be installed as part of the dev-dependencies, no ava config file or .babelrc file is present, the ava config in package.json must require esm, the babel/extensions need to include js and mjs and module compilation needs to be disabled for testOptions/presets:

"ava": { "require": "esm", "babel": { "extensions": [ "js", "mjs" ], "testOptions": { "presets": [ ["module:ava/stage-4", {"modules": false}] ] } } }

It would be great to clean up/clarify the documentation and add specific (verified working) sample skeleton test projects for the most important es6 settings/scenarios for the 1.0.0 final release.

@Siilwyn
Copy link

Siilwyn commented Oct 15, 2018

Remove Babel as a direct dependency. (from #1908 (comment))

This would be delightful for smaller projects, the only reason I'm still choosing tape over ava is the 13 MB install size difference.

@sholladay
Copy link

the only reason I'm still choosing tape over ava is the 13 MB install size difference

As long as it is a devDependency, the size won't affect your users except during development. Also, I'm willing to bet that if you do npm ls ava, you'll find it in your dependency tree already. So npm should be able to dedupe it and install only once.

@Siilwyn
Copy link

Siilwyn commented Oct 15, 2018

It does affect the developer experience though and npm ls ava certainly does not return in my dependency tree for most projects.

@sliekens
Copy link

sliekens commented Nov 19, 2018

Regular user here (with no knowledge of Babel or AVA internals).

I like having my builds completely separate from my tests. That's why I have a babelrc that transpiles sources+tests to an out-dir first. Then I run AVA on the out-dir with require: 'esm', babel: false and compileEnhancements: false. This works really well for me. I can even run babel --watch and ava --watch side by side and I haven't had any issues with that so far (but more experimentation is needed).

edit: getting code coverage to work without polluting the transpiled source files is somewhat of an issue with this setup

edit2: I decided not to have test coverage because of reasons outlined here https://medium.com/the-node-js-collection/rethinking-javascript-test-coverage-5726fb272949

edit3: c8 looks like a promising alternative to nyc that doesn't pollute build output, but it's currently broken on my Windows machine

@sindresorhus sindresorhus pinned this issue Dec 14, 2018
@PM5544
Copy link

PM5544 commented Mar 15, 2019

Our setup uses Rollup to bundle ECMAScript Modules into ESM and ES5 (for unsupported browsers), and we want our tests to also be written in ESM.
After a lot of trial and error the only solution that worked was the one in this comment #1908 (comment)

@novemberborn novemberborn unpinned this issue Sep 15, 2019
@novemberborn
Copy link
Member Author

Please see #2293.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking requires a SemVer major release future we'll get back to this eventually question
Projects
None yet
Development

No branches or pull requests

9 participants