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

Prevent deprecation warning in node 7.4.0 #11

Merged
merged 1 commit into from
Jan 27, 2017
Merged

Prevent deprecation warning in node 7.4.0 #11

merged 1 commit into from
Jan 27, 2017

Conversation

borisdiakur
Copy link
Contributor

Closes #10

@eush77
Copy link
Collaborator

eush77 commented Jan 19, 2017

I think that the callback should check and throw any potential errors instead of just silencing them.

@borisdiakur
Copy link
Contributor Author

borisdiakur commented Jan 19, 2017

@eush77 How about printing a warning as an alternative? This wouldn’t change the current behaviour of the module (at least not drastically).

@eush77
Copy link
Collaborator

eush77 commented Jan 19, 2017

I don't think printing would be wise here since it leaves no way to handle the error and can mess up with the repl prompt. And this module already throw errors related to the history file since it uses openSync and closeSync.

Alternatively, would you feel positive about changing write to writeSync here?

@borisdiakur
Copy link
Contributor Author

@eush77 The fs docs say:

Note that it is unsafe to use fs.write multiple times on the same file without waiting for the callback. For this scenario, fs.createWriteStream is strongly recommended.

So I would actually follow the recommendation and use fs.createWriteStream. What do you think?

@borisdiakur
Copy link
Contributor Author

Ping (mr is ready to merge).

@tmpvar tmpvar merged commit 589410f into tmpvar:master Jan 27, 2017
@tmpvar
Copy link
Owner

tmpvar commented Jan 27, 2017

thanks - published 0.1.4

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.

3 participants