Skip to content
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

Drop Node.js 6, modernize code, return Promise from async function #44

Merged
merged 1 commit into from
May 24, 2019

Conversation

coreyfarrell
Copy link
Contributor

@coreyfarrell coreyfarrell commented May 21, 2019

  • Implement with async functions
  • Use util.promisify
  • Replace var with const or let
  • Use arrow functions for callbacks
  • Update Travis testing matrix

Fixes #33

@isaacs
Copy link
Contributor

isaacs commented May 23, 2019

This is great! Unfortunately, I landed a few PRs before getting to this one, and it's full of merge conflicts.

Would you mind terribly rebasing onto latest master? If that's too much of a pita, I can port it myself pretty soon.

@coreyfarrell
Copy link
Contributor Author

I can probably rebase tomorrow. Could you please look at #33 (comment) let me know what you think about the Promise option?

@isaacs
Copy link
Contributor

isaacs commented May 24, 2019

Go ahead and drop it in the next semver major. Just use and return native promises.

* Implement with async functions
* Use util.promisify
* Replace `var` with `const` or `let`
* Use arrow functions for callbacks
* Remove "node" from testing matrix as this is covered by "12"
* Drop `Promise` option in favor of always using native promises.

Fixes npm#33
@coreyfarrell
Copy link
Contributor Author

OK rebase is done.

One note, the code to get tmpfile into the callback is a little odd now that we the async work is done in a separate function. Would it make sense to have a callback option for reporting creation of the tmpfile:

if (options.tmpfileCreated) {
  options.tmpfileCreated(tmpfile)
}

This would allow tmpfileCreated to be reported as soon as it's created rather than waiting until completion. It would also allow the sync method to report tmpfile even if a failure occurred after the tmpfile was created.

CC @novemberborn

@isaacs isaacs merged commit 5361569 into npm:master May 24, 2019
@isaacs
Copy link
Contributor

isaacs commented May 24, 2019

So far, temp file reporting has never been published in a release, so from my pov, we're free to implement it however. Reporting what temp file was created after it's already been deleted is definitely a bit strange.

Yes, a callback option here makes more sense, imo. Let's get that in and then cut a v3 release.

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

Successfully merging this pull request may close these issues.

Return a promise
2 participants