Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Refactoring #213

Closed
wants to merge 3 commits into from
Closed

Refactoring #213

wants to merge 3 commits into from

Conversation

dima-takoy-zz
Copy link

@dima-takoy-zz dima-takoy-zz commented May 5, 2019

PR included:

  • drop node v6 support (EOL)
  • bump version to 4.2.4
  • updated dependencies
  • replace buble with babel (same perfomance with no restrictions in 2019)
  • start using util.promisify
  • refactor promises with async-await (more perfomant and readable code)
  • drop sync methods ( Use async fs instead of fs.*Sync calls #147 )

All tests pass (thanks for tests!)

@dima-takoy-zz
Copy link
Author

Its PR failed with node v6 because i introduce async-await syntax that not works with node 6.

So its no matter because of node v6 dead. it's EOL = April 2019. (proof: https://github.com/nodejs/Release)

@TrySound
Copy link
Member

TrySound commented May 5, 2019

I would prefer more specific PRs. "Refactoring" is too radical. Version bump will be done before publishing. Also dropping node 6 is a breaking change for those who did not updated yet. Also this project uses npm lockfile, follow it please.

@dima-takoy-zz
Copy link
Author

You're right. I will split this PR into small ones.

@lukastaegert
Copy link
Member

Just wanted to say thanks for the huge effort you are putting into this plugin right now! Just two words from me:

  • Even though Node 6 is EOL, feedback from users supports what @TrySound said. My current perspective would be to target updating the ecosystem to Node 8 maybe for next year, until then compatibility would be nice.
  • Unfortunately as far as I know, this prevents both async-await and utils.promisify for the time being

@lukastaegert
Copy link
Member

However I like the remaining changes, i.e. updating the dependencies, replacing buble with babel and replacing the sync methods!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants