search

avajs / ava

Node.js test runner that lets you develop with confidence 🚀
MIT License
20.73k stars 1.41k forks source link

Consider adding babel-core to a peerDep like other integrations (+ to @babel/core) #1575

Closed hzoo closed 6 years ago

hzoo commented 7 years ago

Feature Request

We are going to release Babel 7 soon, and right now it's incompatible with ava since it's a peerDep (the dependency needs to be updated either way). I understand that it is nicer to have it as a dependency though because it means less installing.

babel-loader has had babel-core as a peerDep before, and I moved grunt/gulp/rollup/cli to it as well in v7 beta. The reasoning was just that people were installing v7/v6 in their dependencies. Peer dependencies can be pretty annoying as well (warnings especially if it's a transitive dependencies) but it seems to make sense for babel-core and we will try it out? Having to ask this for all the integrations does make me not want to do it though 😛

Also we are going to move to scoped packages soon!

Or is there an option to use a different version already?

loganfsmyth commented 7 years ago

Another way to phase this is, how do we move forward without Ava being tied aggressively to Babel 6.x, peerDep or otherwise? Once we release 7.x I'd presume that most users will start migrating to it, and Ava will likely be a big sticking point since it will try to use Babel 6, and other parts of user's build process will try to use Babel 7.

On the Babel side of things, we're planning to release new major versions of all of our main integration packages with

"peerDependencies": {
  "@babel/core": "7.x"
}

The downside for ava being that users who don't care about Babel would get a peerDep warning about it when they install, but maybe that is alright?

novemberborn commented 7 years ago

Hi @hzoo and @loganfsmyth, thanks for reaching out.

I think for the time being AVA will depend directly on Babel, as one of our selling points is that you get stage-4 features across Node.js versions out of the box. That said we can inspect the package.json to see if a newer Babel version has been installed in the project itself.

Perhaps not immediately but eventually we'll switch to depending on Babel 7 directly, and falling back to Babel 6 if it's installed in the project being tested.

We'll make sure our own presets are compatible with both versions, and we'll have to update our config resolution to support the new .js files.

Will babel-core@latest see a v7 release, or will this solely be @babel/core?

loganfsmyth commented 7 years ago

Fair enough. As people switch to v7 you'll likely start seeing issues from users who use "babel": "inherit" since that will try to load Babel 7 plugins into Ava's Babel 6 core.

We'll have to update our config resolution to support the new .js files.

I don't think I realized Ava has its own implementation of our config resolution. I'd love to talk eventually to see if any of those performance improvements could be built into Babel's core itself to avoid you needing the separate implementation.

Will babel-core@latest see a v7 release, or will this solely be @babel/core?

The plan at the moment is to have @babel be Babel's new home. The only thing I've proposed, which isn't guaranteed, is that we could technically make a babel-core package that had its own peerDep on @babel/core as kind of a bridge so packages could write a peerDep on "babel-core": "6.x | 7.0-bridge" rather than making packages introduce a breaking change to remove babel-core with @babel/core, but not decided on that.

novemberborn commented 7 years ago

Fair enough. As people switch to v7 you'll likely start seeing issues from users who use "babel": "inherit" since that will try to load Babel 7 plugins into Ava's Babel 6 core.

Right, but I'm thinking we'll detect when people have v7 installed and use that instead. Then a little while after v7 has come out and the wider ecosystem has had a chance to adjust we can switch to run v7 by default (and perhaps detect and use v6 when installed).

I don't think I realized Ava has its own implementation of our config resolution. I'd love to talk eventually to see if any of those performance improvements could be built into Babel's core itself to avoid you needing the separate implementation.

Yup, happy to talk. There's a variety of (somewhat subtle) reasons. The new dynamic configuration files do make it trickier though 😉

The plan at the moment is to have @babel be Babel's new home. The only thing I've proposed, which isn't guaranteed, is that we could technically make a babel-core package that had its own peerDep on @babel/core as kind of a bridge so packages could write a peerDep on "babel-core": "6.x | 7.0-bridge" rather than making packages introduce a breaking change to remove babel-core with @babel/core, but not decided on that.

That sounds great.

sindresorhus commented 7 years ago

Right, but I'm thinking we'll detect when people have v7 installed and use that instead. Then a little while after v7 has come out and the wider ecosystem has had a chance to adjust we can switch to run v7 by default (and perhaps detect and use v6 when installed).

Seems like a lot of extra work and complexity for little gain. Can't we just upgrade to Babel 7 in a new AVA version and people that can't upgrade to Babel 7 yet can just wait on upgrading AVA?

novemberborn commented 7 years ago

Can't we just upgrade to Babel 7 in a new AVA version and people that can't upgrade to Babel 7 yet can just wait on upgrading AVA?

We could, yes. I haven't looked enough into the changes to venture a guess as to how disruptive it'll be.

loganfsmyth commented 7 years ago

Unless I'm misunderstanding, the conflict only comes up when users use inherits for their config. Maybe at Ava could check if the version installed in the user's project is the same one used by Ava, and warn them?

nervetattoo commented 6 years ago

Judging by this thread, I get the impression that using babel7 should just work because ava brings its own babel dep. However, I made an attempt to use babel 7 in a project that uses ava for testing of react components, but even when using just the @ava/stage-4 babel preset and no inherits of babelrc it fails due to loose option being wrong somewhere. I didn't look further into it as I just wanted to test out babel7, and wasn't ready to invest time in issues since its still in beta. Just chiming in that it certainly didn't work out of the box at least :)

loganfsmyth commented 6 years ago

@nervetattoo If you can put together a reproduction repository example, I can take a look. Definitely not what I'm expecting to happen.

vjpr commented 6 years ago

@loganfsmyth Here is the loose error:

  TypeError: Cannot read property 'loose' of undefined
    at _default (/app/babel7-test/node_modules/.registry.npmjs.org/@babel/plugin-proposal-optional-chaining/7.0.0-beta.32/node_modules/@babel/plugin-proposal-optional-chaining/lib/index.js:13:32)
    at Function.memoisePluginContainer (/app/node_modules/.registry.npmjs.org/babel-core/6.26.0/node_modules/babel-core/lib/transformation/file/options/option-manager.js:113:13)
    at Function.normalisePlugin (/app/node_modules/.registry.npmjs.org/babel-core/6.26.0/node_modules/babel-core/lib/transformation/file/options/option-manager.js:146:32)
    at /app/node_modules/.registry.npmjs.org/babel-core/6.26.0/node_modules/babel-core/lib/transformation/file/options/option-manager.js:184:30
    at Array.map (<anonymous>)
    at Function.normalisePlugins (/app/node_modules/.registry.npmjs.org/babel-core/6.26.0/node_modules/babel-core/lib/transformation/file/options/option-manager.js:158:20)
    at OptionManager.mergeOptions (/app/node_modules/.registry.npmjs.org/babel-core/6.26.0/node_modules/babel-core/lib/transformation/file/options/option-manager.js:234:36)
    at OptionManager.init (/app/node_modules/.registry.npmjs.org/babel-core/6.26.0/node_modules/babel-core/lib/transformation/file/options/option-manager.js:368:12)
    at File.initOptions (/app/node_modules/.registry.npmjs.org/babel-core/6.26.0/node_modules/babel-core/lib/transformation/file/index.js:212:65)
    at new File (/app/node_modules/.registry.npmjs.org/babel-core/6.26.0/node_modules/babel-core/lib/transformation/file/index.js:135:24)
    at Pipeline.transform (/app/node_modules/.registry.npmjs.org/babel-core/6.26.0/node_modules/babel-core/lib/transformation/pipeline.js:46:16)
    at CachingPrecompiler._transform (/app/node_modules/.registry.npmjs.org/ava/0.22.0/node_modules/ava/lib/caching-precompiler.js:58:24)
    at transform (/app/node_modules/.registry.npmjs.org/caching-transform/1.0.1/node_modules/caching-transform/index.js:43:10)
    at CachingPrecompiler.transform (/app/node_modules/.registry.npmjs.org/caching-transform/1.0.1/node_modules/caching-transform/index.js:60:17)
    at CachingPrecompiler.precompileFile (/app/node_modules/.registry.npmjs.org/ava/0.22.0/node_modules/ava/lib/caching-precompiler.js:39:9)
    at Api._runFile (/app/node_modules/.registry.npmjs.org/ava/0.22.0/node_modules/ava/api.js:58:33)
From previous event:
    at Api._runWithPool (/app/node_modules/.registry.npmjs.org/ava/0.22.0/node_modules/ava/api.js:243:5)
    at _setupPrecompiler.then.then (/app/node_modules/.registry.npmjs.org/ava/0.22.0/node_modules/ava/api.js:174:17)
    at <anonymous>
loganfsmyth commented 6 years ago

I should clarify what "works with Babel 7" means in this context. At the end of the day, Ava has 100% tied itself to Babel 6, so any config you pass to Ava needs to be config for Babel 6.x.

You could install Babel 7 locally and build your own code with it if you want, but Ava itself doesn't know how to handle Babel 7 stuff. It looks like you've either passed babel: 'inherits' or explicitly passed @babel/plugin-proposal-optional-chaining as a plugin for Ava to load, which won't work. You'd have to use something like @babel/cli before Ava runs.

vjpr commented 6 years ago

Its unfortunate that ava is so closely tied with babel and babel@6. Especially with its own config resolver. I feel its way too complex. Ava should remove all dependencies from babel, and add them to a separate package called ava-babel-6 and ava-babel-7.

I'd really like to use the new existential soak and ava is the only blocker in my codebase.

vjpr commented 6 years ago

So I got ava to run with babel@7 + @babel/plugin-proposal-optional-chaining quite easily.

Using {babel: inherits}...

ava/lib/caching-precompiler.js:48

this.babel = require('@babel/core');
//this.babel = require('babel-core');
//inputSourceMap: getSourceMap(filePath, code),
novemberborn commented 6 years ago

I've posted the plan in #1598. Thanks all.

dacz commented 6 years ago

to @vjpr solution (thanks for it, because I'm AVA man): it works but it seems to be more complex (and more babel6 wired). I wanted to use t.true and got

Uncaught Exception: utils/objectTemplateCheck.test.js
  undefined
  TypeError: file.parse is not a function
    at doParse (/Users/dacz/development/sea/node_modules/babel-plugin-espower/lib/babel-assertion-visitor.js:164:27)

Fortunately t.is(got, indefined) works.

loganfsmyth commented 6 years ago

@dacz It looks like that plugin is using an undocumented API from Babel 6 that was removed in v7. It looks like a fix has landed in https://github.com/power-assert-js/babel-plugin-espower/commit/e56edd188ffd4c9ceef4f77b59b3c39962355eb5#diff-b75a7345fb93215f800539fa526b9d23 but there's no version on npm with that available. Maybe worth asking them what their plan is.

dacz commented 6 years ago

@loganfsmyth I think you are right that this is not a problem with babel, but AVA with Babel 6 too much under it's skin. I hope that AVA will sort this out, it's excellent tool.

novemberborn commented 6 years ago

I think you are right that this is not a problem with babel, but AVA with Babel 6 too much under it's skin. I hope that AVA will sort this out, it's excellent tool.

@dacz AVA does use some plugins that, as you found, were written for Babel 6. You can follow #1598 for progress on making AVA work solely with Babel 7 instead.

loganfsmyth commented 6 years ago

@novemberborn @dacz We've definitely tried to prevent breakage. There's one issue with babel-plugin-istanbul in https://github.com/istanbuljs/istanbuljs/issues/92 that I know of that they'll have to fix. That said, if you do run into plugins that are failing, do feel free to report it to us. I have purposefully removed a few things, but I've tried to keep it to things that I honestly hoped no-one was using, like that .parse function.

dacz commented 6 years ago

@novemberborn @loganfsmyth thanks for info. I appreciate and I'm grateful for amazing work on Ava. I saw branch babel7 in repo. Is there npm beta package we can try and test or only via clone?

novemberborn commented 6 years ago

@dacz you'll have to install from GitHub, yes. It should work with npm install avajs/ava#babel7.