Refactored with async/await

[Zearin]

Summary

I have refactored Metalsmith to use async/await.

I’m looking for feedback. What do you think?

Motivations

• ES6 class. I find these classes much easier to read, write, and understand.

  • Notes: Although it was temping to convert methods like clean() to use getters and setters, I did not alter their current API. No arguments returns the value, and passing arguments sets the value.

• ES6 const/let. I love the certainty that const provides. I love the predictable scope that let provides. (I never liked how var lends itself to occasionally surprising and hard-to-debug behavior, or “clever” code that intentionally leverages such behavior, which is a barrier to understanding the code quickly.)

• async/await. I find async and await code easier to read, write, and understand. It reads top to bottom, clearly marks which operations are asynchronous, and make normal use of try…catch.

  • Notes (generators): I think generators are cool, and simple to understand in principle…but sometimes difficult to understand in practice. Switching to async/await also removes the need for unyield and thunkify dependencies, which both made it harder for me to understand how Metalsmith worked (although they are also cool, and allowed unbelievable performance gains in Metalsmith 2).
  • Notes (promises): I similarly feel that Promises are simple to understand in principle, but often difficult to understand in practice. Nesting and callbacks force me to constantly read back and forth through scopes to understand what’s going on, instead of top to bottom like plain, “boring” synchronous code. The fact that async/await is built on Promises while allowing such a radically simpler code structure is pretty fantastic. ☻

I haven’t converted all the syntax (i.e. I’m still using require() and module.exports instead of import and export.

Last Remarks

• No changes to Metalsmith’s API.

• All existing tests pass. (…Except one! I couldn’t get the “no new required” test to pass. However, you can definitely omit new when calling Metalsmith(), and all the other tests do so.)

• Performance seems to be about the same (But I am not an expert at performance analysis/ benchmarks/metrics, or any of that other math-y stuff. If you are good at that stuff, I’d love to get performance data that’s more detailed than what the test suite shows.)

---

If this is a good candidate for Metalsmith v3.0, let me know.

[Ajedi32]

Wow, nice work! There's a lot of changes here and I haven't yet done a thorough review, but it does indeed look like this could become the basis for the next major release (though there's probably still a bit more work that needs to be done before then).

I've been meaning to migrate to an promise-based API for a while now, but there have been some concerns about backwards-compatibility (see #231, #231). While I'm okay breaking some functionality in a semver-major release, I'd still prefer to retain backwards compatibility as much as reasonably possible to avoid creating unnecessary work for our users. Migration should be as quick and painless as possible. I especially want to avoid breaking plugins to ensure we don't fragment the ecosystem.

Just to clarify, when you say

No changes to Metalsmith’s API

Do you mean aside from the change to a Promise-based API? Or are you saying existing Metalsmith applications are actually fully compatible with these changes and can upgrade without having to modify any of their code?

[Zearin]

Do you mean aside from the change to a Promise-based API?

Ah, nice catch! You are correct. This will still require changing code.

I approached the changes with the (remote) hopes of something for v3.0, so I was okay with some small changes. However, when you say:

I'd still prefer to retain backwards compatibility as much as reasonably possible to avoid creating unnecessary work for our users. Migration should be as quick and painless as possible.

I totally agree, and I tried to make this the case as well.

I guess the main requirement for existing code would be:

• Add await to method calls (except the pseudo-getters/setters); OR
• Use .then(…) / .catch(…) for method calls (except the pseudo-getters/setters)

The most visible such change will be to metalsmith.build(). When working to get some of the CLI tests to pass, I ran into the limitation of not being able to use await metalsmith.build() at the root level of a program.

The solution turned out to be pretty simple! I just moved the old callback into .then(…), and pulled out the one-line error handler—which is typical of most existing .build() callbacks—into a small .catch(…). Done.

In the tests, plugins that use the existing signature (files, metalsmith, done) pass the tests with no changes. So, existing plugins should be fine.

I don’t think these changes are asking a lot for a semver-major upgrade. I’m pretty sure that only code which makes heavy use of Metalsmith’s other methods (non pseudo-getters/setters) will have to put in more than a few minutes' effort.

If this makes it into v3.0, I can also write up a quick guide for migrating from Metalsmith 2 to Metalsmith 3 code.

[Zearin]

(Aside: I think this sort-of address #211. When calling build() with await, no arguments are required.)

[leviwheatcroft]

In principle, I think this is a good move. would be great to get rid of unyield & thunkify.

I haven't had time to look through, but my first questions are:

• what changes would this require for plugins to remain compatible? I see we're still using Ware to manage the plugins. So for those plugins that strictly adhere to the api, no changes would be required I guess.
• bumping a major version is an opportunity to sneak in some other features. That said, there's none I'm aware of. Is there anything else we might include in a major ?
• eslint:recommended is fairly generic, might we switch to something more generic ?

[Zearin]

• what changes would this require for plugins to remain compatible? I see we're still using Ware to manage the plugins. So for those plugins that strictly adhere to the api, no changes would be required I guess.

I don’t remember having to mess with the plugins. Most of the work was involved Metalsmith’s methods (and the tests’ calls to them).

• bumping a major version is an opportunity to sneak in some other features. That said, there's none I'm aware of. Is there anything else we might include in a major ?

I’m for #205 (separate CLI). If you are up for it, I can bump my branch version to something like 3.0.0-pre-alpha (or whatever suffix you like), and you could create a branch in the main repo where

I would also like to use JSDoc to auto-build some API docs, if you are up for it. (These can never—and should never—replace human-written docs such as guides, tutorials, etc. But API docs are useful in their own way. Also, JSDoc’s default HTML styles are ugly—but fear not; I would beautify them! ☻)

• eslint:recommended is fairly generic, might we switch to something more generic ?

Dunno. I just started using ESLint, but I love it. (I also love the helper [IanVS/eslint-nibble]. Great for inspecting and then auto-fixing single rules at a time—like semicolons, spacing, etc. Highly recommended.)

So if eslint:recommended is “fairly generic”, what did you have in mind when you ask about “something more generic”?

[leviwheatcroft]

my bad I meant something "less" generic.

Does this change fix #211 (silent fail on build with no args)?

[Zearin]

Does this change fix #211 (silent fail on build with no args)?

…Sort of. :)

One of the following must be done:

let files = await metalsmith.build()

OR (since it’s currently impossible to await at the top level of a script):

metalsmith.build()
.then(  (files) => { /* handle success */ })
.catch( (err)   => { /* handle error */   })

There’s actually an async-flavored IIFE technique (which I found here):

(async () => {
    let files = await metalsmith.build();
})();

When I was refactoring to async, I got lots of warnings from Node along the lines of "Unhandled Promise rejections are deprecated", and that it would be a fatal error in future Node releases. As a result, there are try/catch blocks in Metalsmith’s async methods (including metalsmith.build()). I don’t think there should be any silent errors. (Most catch blocks just throw err, and allow calling code to handle it as desired.)

[Zearin]

I’ve pushed a few small changes to my branch, and marked it as 3.0.0-alpha.0.0.1.

Should I open a Pull Request? (That will make it easier to compare code, comment on specific bits of code, and be aware of further commits.)

If I open a Pull Request, let’s add a reference to this Issue at the top, and continue the conversation there.