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

Return a promise #33

Closed
MajorBreakfast opened this issue Mar 12, 2018 · 7 comments · Fixed by #44
Closed

Return a promise #33

MajorBreakfast opened this issue Mar 12, 2018 · 7 comments · Fixed by #44

Comments

@MajorBreakfast
Copy link

Since the library is already written with promises, it'd be nice if it could return a promise. I generally prefer promises over callbacks, because they're really nice to work with in conjunction with async functions 😋

@iarna
Copy link
Contributor

iarna commented Jan 23, 2019

Agreed, I would love a patch that makes it behave like Bluebird's asCallback method, eg, hybrid mode where a promise is always returned and a callback is used if one is provided.

@coreyfarrell
Copy link
Contributor

@iarna what is the node.js support policy for this module? Specifically are you open to a breaking change to require node.js 8? This would allow use of util.promisify and async/await making it very simple to implement a promise API which still supported the callback argument. I know it's a big jump from the currently supported versions so I understand if this idea is not acceptable.

@iarna
Copy link
Contributor

iarna commented Apr 11, 2019

@coreyfarrell This module currently supports node 4 up. Removing things from that list is a breaking change. The npm cli support policy is "any version supported by the Node Foundation", AND the list can only change on major version bumps. This seems like a generally solid rule to apply for libraries as well. Node 6 doesn't drop out of support until April 30th. After that date, a Node 8+ version could be released under a new major.

However, I should note that I will not be in a position to 👍 or 👎 changes like this when that date comes.

@coreyfarrell
Copy link
Contributor

I hadn't realized they bumped the date, I thought April 1st was the EOL date (nodejs/Release#421).

In any case I wish you the best on whatever comes next for you!

@sindresorhus
Copy link

Node 6 doesn't drop out of support until April 30th. After that date, a Node 8+ version could be released under a new major.

That date has passed.

coreyfarrell added a commit to coreyfarrell/write-file-atomic that referenced this issue 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, add Windows testing

Fixes npm#33
coreyfarrell added a commit to coreyfarrell/write-file-atomic that referenced this issue 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 npm#33
@coreyfarrell
Copy link
Contributor

I've opened #44. One thing that I'm not sure about is the Promise option. In my current patch this option is still honored for the per file serialize logic only. So a custom promise is only used for that one thing, everything else uses native promises. I feel like my PR makes the Promise option fake. Should support for custom promises be removed?

@isaacs
Copy link
Contributor

isaacs commented May 24, 2019

I think if we're going to drop support for Node.js 6 in a semver major update, then dropping custom promises makes sense. Native promises are very good now.

coreyfarrell added a commit to coreyfarrell/write-file-atomic that referenced this issue May 24, 2019
* 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
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 a pull request may close this issue.

5 participants