Setup babel

[AdrieanKhisbe]

Implement #207 \o/

(might even be working on node 0.10)

[AdrieanKhisbe]

(about the build -> this is just coveralls complaining)

[Ajedi32]

Woah. Isn't this breaking backwards compatibility if we change the way Metalsmith is imported?

[AdrieanKhisbe]

I discovered afterward that this was a bug from babel 6, and just foud this workaround:
https://github.com/59naga/babel-plugin-add-module-exports

[AdrieanKhisbe]

@Ajedi32 .
Et voilà :)

[Ajedi32]

I think this change from your previous PR got mixed in by mistake...

[AdrieanKhisbe]

or maybe, I should test if it works on 0.10, and then add it.
No?

[Ajedi32]

Yeah, if you want to go ahead and add Node 0.10 to the Travis build, and if the tests pass we can make that the minimum Node version.

[AdrieanKhisbe]

0.10 not working because of co-fs-extra https://github.com/wxygeek/co-fs-extra

I consider that wouldn't be bad to drop this dependency.
your call!

[Ajedi32]

Yep. Looks like one of our dependency's dependencies uses generator functions. 😉

I actually do plan to replace co with async/await after we merge this PR anyway (along with a bunch of other nice refactorings enabled by ES6/7), so if you want to try dropping that dependency now, go for it.

Or alternately if doing that proves difficult, just change the minimum Node version to 0.12.0 (and either stop testing 0.10 in Travis, or add it as an allowed failure) and I'll restore 0.10 support as part of my refactoring.

[Ajedi32]

One more thing we probably want to consider before merging this PR is source maps for our test coverage reports. Right now Coveralls is showing this: https://coveralls.io/builds/4727897/source?filename=lib%2Findex.js

We might be able to get this working by turning on source maps in Babel and upgrading to Istanbul 1.0.0-alpha.2, based on this comment: gotwarlost/istanbul#212 (comment)

[Ajedi32]

You might be able to tweak this to be a bit more elegant and more efficient on repeated builds. make can be a surprisingly powerful tool in situations like this. Definitely not a big deal, but I'll look into it later today.

[AdrieanKhisbe]

The async await will require some babel to be set up.
I can take care of this, so you can build up the refactor on this.

Since we are gonna throw co anyway with the async await, I'll prefer
no waste time to tweak it so we have it working without co-fs-extra.

---

About coverage.
I'm a bit relucting to add a dep on a alpha. prefer the stable to land in.

[Ajedi32]

Yeah, I'd normally be a bit nervous about using an alpha prerelease too, but it is only a devDependency, and it's only used for generating test coverage reports, not for building Metalsmith,

This isn't something we have to worry about causing problems for Metalsmith's users if we use it.

[Ajedi32]

Alternately, we could try switching to a tool like isparta.

[AdrieanKhisbe]

It's more on the point, having to readadapt changes they made up to 1.0.
(so far I have no visibility how big they would be)

about isparta dunno. never heard about it and there seems to be some major pending issues

[Ajedi32]

Yeah, fair enough. It would be kind of annoying if something broke while upgrading from the alpha to the stable release.

On the other hand though, assuming we don't have to change anything while upgrading from 0.4.1 to 1.0.0-alpha.2, then any changes we have to make later while upgrading to the stable version of 1.0.0 after its release will be changes we'd have had to make anyway, right?

The way I see it, we have basically four options here:

1. Merge the Babel upgrade without source mapped test coverage
   • We'd still have test coverage metrics, but there'd be no way to view which lines are covered in Coveralls
   • I personally dislike this option, as it feels kinda sloppy to me to break things and "fix it later"
2. Wait for the Istanbul 1.0 release, then get source maps working and merge this
   • Seems like an okay option, but I really have no idea how long we'll be waiting if we decide to do this. Istanbul v1.0.0-alpha.2 came out almost three months ago, and there hasn't been a stable release yet.
3. Use Istanbul v1.0.0-alpha.2 to get source mapped test coverage
   • Pros and cons of this discussed above
4. Use a different tool for test coverage reports
   • I'm fine with this option, but we need to find a good replacement tool first

Assuming we can get source mapped test coverage working with Istanbul v1.0.0-alpha.2 and don't notice any major problems with it I personally feel like that's the best option. That's certainly up for debate though. If you have an alternate solution you want to pursue please let me know.

[Ajedi32]

Also, one other change I just realized this PR needs is an .npmignore file: https://docs.npmjs.com/misc/developers#keeping-files-out-of-your-package

If we don't include that, npm won't include any of the transpiled destination files in the published package (since those folders are in .gitignore).

[Ajedi32]

And oh yeah, I almost forgot. For option 4, Lab is a possibility (#215) as it supports source maps for test coverage. It doesn't support promises though hapijs/lab#79, which could get kind of annoying once I start refactoring code to use ES6 features like async/await (which are basically just sugar over promises).

[AdrieanKhisbe]

I'll tend more on option 4 and lab, but might be biased.

About async/await, start to look.
I set up babel, but aven't start converting yet.
But maybe should consider twice befaore to go to async:
cf https://spion.github.io/posts/es7-async-await-step-in-the-wrong-direction.html.

[AdrieanKhisbe]

@Ajedi32 what do you want we add to the .npmignore ?

[Ajedi32]

@AdrieanKhisbe Yeah, I've read articles like that before. They definitely make good points, but so far I'm not really convinced.

In this particular instance, that article seems to be arguing not only against async/await, but also generator functions, which we're already using. Additionally, the alternative it recommends, just using promises directly, is fully compatible with the use of async/await. (Since it's basically just sugar on top of using Promises.) So overall it's not something I think we need to worry about too much, since you can always just use async/await when you can and switch to Promises where you need them. It is definitely something I plan to keep in mind though in case I start running into similar problems to those mentioned in the article.

And ah, I didn't realize we already had an .npmignore. Nevermind then. I was just concerned about the behavior where Node uses .gitignore by default if no .npmignore file exists, since our build directories are now included in .gitignore.

[Ajedi32]

Ah, in the second half of the article it looks like they're arguing generators are more powerful than async/await since they lets you define your own execution engine. Missed that part. I agree that is true, but on the flip side there's also some degree of complexity that goes along with that. In Metalsmith we've solved that problem by outsourcing our execution engine to the co library, which basically just implements async/await on top of generator functions. We do this because, right now, we don't need to implement our own execution engine. async/await will serve a similar purpose to co without requiring the extra dependency (and, IMO, it's a lot easier to read and understand).

[Ajedi32]

So to summarize the current changes needed to this PR, we need to:

• Revert (or remove) 1d5a37f (temporarily)
• Change the minimum node version to "node": ">=0.12.0" (either here or in Explicitely state minimal node engine #210)
• Configure Babel to generate source maps
• Figure out how to get source maps working with Coveralls. (See my previous comment above.)

[ben-eb]

@Ajedi32 It might be wise to switch to a files key in package.json instead of an .npmignore - an explicit whitelist of package files is a lot easier to maintain than a blacklist.

https://github.com/ben-eb/postcss-color-yiq/blob/master/package.json#L11-L14

Sourcemap configuration with babel is pretty easy; I use this setup:

{
  "presets": ["es2015-loose", "stage-0"],
  "plugins": ["add-module-exports"],
  "env": {
    "development": {
      "sourceMaps": "inline"
    }
  }
}

https://github.com/ben-eb/postcss-color-yiq/blob/master/.babelrc

You can ignore sourcemaps when not in development by passing a different environment to babel;

https://github.com/ben-eb/postcss-color-yiq/blob/master/package.json#L8

For testing I use AVA and nyc for all new projects, works great so far with coveralls. I wrote a little guide that might be of use:

https://github.com/sindresorhus/ava/blob/master/docs/recipes/code-coverage.md

Here's the same project on coveralls:

https://coveralls.io/github/ben-eb/postcss-color-yiq

Hope this helps!

[AdrieanKhisbe]

It might be wise to switch to a files key in package.json instead of an .npmignore - an explicit whitelist of package files is a lot easier to maintain than a blacklist.

I do agree. Will do

Thanks @ben-eb to the tips and pointers. will dig into this and apply in the week. :)

[Ajedi32]

@ben-eb Thanks, that's definitely helpful. My current thoughts on this:

1. Agreed on using files over .npmignore.
2. It's probably a good idea to avoid using different Babel options in the prepublish step, since prepublish is run on local npm install (in development, where you'd probably want to have source maps). If we want to exclude source maps from the published package (Do we? Could they possibly help our users with debugging? Idk...), I'd prefer to instead put the source maps in separate files (e.g. not inline) and exclude those files from the published package (with the files property in package.json).
3. Using AVA has the same problem for Metalsmith as switching to Lab; we'd be changing our entire test framework just to keep test coverage reports working. I don't necessarily have a problem with switching test frameworks (and Ava actually looks pretty nice), but if at all possible I'd prefer to propose major changes like that in a dedicated PR.

Regarding what tool to use to generate source mapped coverage reports (for now anway; once again we can always switch later if we ultimately switch over to a different test framework), I still think using the istanbul alpha release is a good option, but I'd also accept a solution using isparta, babel-istanbul, or another tool designed for this purpose.

Isparta seems to be a very popular choice at the moment. A little Googling for ES6 test coverage reveals tons of blog posts about solving this problem and most of the ones I've seen so far use isparta. (Example: https://onsen.io/blog/mocha-chaijs-unit-test-coverage-es6/ )

[woodyrew]

@ben-eb Use of files is a great idea and a good example. package.json files definition for the knowledge.

@Ajedi32 It might be wise to switch to a files key in package.json instead of an .npmignore - an explicit whitelist of package files is a lot easier to maintain than a blacklist.

[ben-eb]

@Ajedi32 Unless I'm mistaken (I don't find a use for source maps most of the time) source maps are only necessary in the tests for istanbul to accurately map the covered lines of ES5 back to the original ES2015. So my setup will run prepublish after npm install, but it will also run babel again when testing, keeping the sourcemaps. I don't see that there is a need to keep them in for the end consumer of the module, but if you do you can always move that config from the env key to the root.

Agreed that any consideration for another test runner should be considered a separate issue as it would be a big change. (Note that nyc is basically istanbul with sub-process support, however).

[Ajedi32]

I think source maps can be useful in development as they allow things like remapping stack traces to the source code, displaying source instead of transpiled code in debuggers and things like that. (I don't know how much of that functionality node has built-in, but it looks like there's a module for stack trace support at least.) That's why I suggested it might be useful to leave them in for development (and possibly in the published package too, though it's debatable whether end users would even bother to try debugging errors originating in transpiled Metalsmith code).

[ben-eb]

@Ajedi32 Yeah, that's how I have it set up too; the babel environment defaults to development, where source maps are used. Only on npm publish are the source maps removed.

[Ajedi32]

@ben-eb Right, but prepublish also gets run on npm install, which is something you normally do in development. 😕

I guess as long as you always manually rerun babel after npm install (or rerun the tests) then that's not an issue, but it does seem like potentially confusing behavior for npm install to nuke your source maps.

[AdrieanKhisbe]

I am a bit busy theses days.

Just discovered https://github.com/normalize/mz with nzgb post: https://twitter.com/nzgb/status/693081182158790658

Might be a cool think to use this in refactor! (easily simplifying things!)

(I'll come back reread in depth previous comment when I'll have more bandwith 😃 )

[Ajedi32]

Yeah, switching to a Promise-based API for the Node core methods that we're using is definitely something that should happen once we switch to ES6.

[RobLoach]

switching to a Promise-based API for the Node core methods that we're using is definitely something that should happen once we switch to ES6.

Promises are available in Node 4 stable, without the need of Babel.

[Ajedi32]

@RobLoach True, but as of right now we're still supporting Node 0.10. Using Babel would allow us to continue supporting Node 0.10 while also using promises.

Additionally, the advantages of consuming a promise-based API are much more pronounced once you have the ability to use async/await, which isn't available in Node 4 without Babel.

[AdrieanKhisbe]

@Ajedi32 do we really really need the async await?
to me benefits are totaly outweighted by the cost

[Ajedi32]

@AdrieanKhisbe What costs are you referring to? You mean the overhead of Babel and difficulty of implementation?

async/await isn't the only ES6 feature which would be made available by Babel FYI, there's a whole list of them here: https://kangax.github.io/compat-table/es6/ (Compare the Node 4.x column with the Babel column). async/await is just the feature I think we'd see the most benefit from.

[AdrieanKhisbe]

I I mean in tooling, maintenance issue, and other code related.
async/await it not gonna land this year, and the api is far to be frozen.

Not a fan of unyieldand yield and trying to hide the asynchronism.
to me callback and promise, in the context, are not a big deal that requires to used this uncertain language feature.

but it's not my call :)
here is some usefull link: https://ponyfoo.com/articles/understanding-javascript-async-await

[AdrieanKhisbe]

Closing because in conflict, outdated