-
Notifications
You must be signed in to change notification settings - Fork 628
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
Refactored with async/await #303
Comments
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
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? |
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 totally agree, and I tried to make this the case as well. I guess the main requirement for existing code would be:
The most visible such change will be to The solution turned out to be pretty simple! I just moved the old callback into In the tests, plugins that use the existing signature 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. |
(Aside: I think this sort-of address #211. When calling |
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:
|
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).
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! ☻)
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 |
my bad I meant something "less" generic. Does this change fix #211 (silent fail on |
…Sort of. :) One of the following must be done: let files = await metalsmith.build() OR (since it’s currently impossible to 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 |
I’ve pushed a few small changes to my branch, and marked it as 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. |
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.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 thatconst
provides. I love the predictable scope thatlet
provides. (I never liked howvar
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 findasync
andawait
code easier to read, write, and understand. It reads top to bottom, clearly marks which operations are asynchronous, and make normal use oftry…catch
.async
/await
also removes the need forunyield
andthunkify
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).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()
andmodule.exports
instead ofimport
andexport
.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 omitnew
when callingMetalsmith()
, 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.
The text was updated successfully, but these errors were encountered: