Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Adding support for .node_history and .noderc.js in $HOME and `pwd` #3178

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet

This adds support for .node_history, which behaves like a .bash_history for the node repl. When an interactive node quits, it writes ~/.node_history and will load it the next time you start the node repl.

This patch also allows a user to create a file called .noderc.js and place it in their home directory, or the directory they start node from.

When node is started with the -i flag or without arguments, it fires up the REPL.
At this point it will check if ~/.noderc.js or <pwd>/.noderc.js exist. If they do, whatever objects they export will be copied into the repl context and be available for use.

A common use case for this will likely be loading debugging tools. For example I have the following as my noderc:

var util = require('util');

function inspect(obj) {
  console.log(util.inspect(obj, true, 10, true));
}

exports.p = inspect;

That way whenever I start the node repl, I can just type p({some:'huge object'}) without having to retype basic boilerplate every time I start node.

+1 for that! But, why don't you just omit the .js extensions? Never saw an rc files with a file extension..

Adding the file extension seems like it would alleviate possible confusion, as well as play nice with editors. I'm not 100% sold on leaving the extension there, but is seems like the rational thing to do.

I really like this idea. +1

That last commit adds a history feature to the node REPL that behaves similiar to a bash history.

milani commented Apr 27, 2012

+1

Sannis commented Apr 28, 2012

That last commit adds a history feature to the node REPL that behaves similiar to a bash history.

Maybe it is better to chnage JSON format for history file for something more standard? grep-ing history should be allowed

@bnoordhuis bnoordhuis and 2 others commented on an outdated diff Apr 28, 2012

+ fs.readFile(nodeHistory, function(err, data) {
+ if (err) return;
+ try {
+ var history = JSON.parse(data);
+ repl.setHistory(history);
+ } catch (e) { }
+ });
+ }
+
+ function saveReplHistory(repl, callback) {
+ var fs = NativeModule.require('fs');
+ var history = JSON.stringify(repl.getHistory(), false, 1);
+ if (!nodeHistory) return callback();
+
+ fs.writeFile(nodeHistory, history, function(err) {
+ callback();
@bnoordhuis

bnoordhuis Apr 28, 2012

Owner

This swallows errors. Is that intentional?

@regality

regality Apr 29, 2012

Yes, if someone doesn't want a history and does a chmod 000 ~/.node_history then we don't want to complain about it. Are there any use cases where we want to display errors when saving the history? I wouldn't mind it being a only displayed with a verbose flag, but I'm not aware of node having verbose logging at this time?

@Sannis

Sannis Jun 7, 2012

We can show warning to user in this case, like it done for npm * usage in repl.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Apr 28, 2012

@@ -123,11 +124,37 @@
if (parseInt(process.env['NODE_DISABLE_COLORS'], 10)) {
opts.useColors = false;
}
+
+ var home = process.env.HOME;
@bnoordhuis

bnoordhuis Apr 28, 2012

Owner

This won't work on Windows.

@regality

regality Apr 29, 2012

Damn wikipedia. Actually fixed in 61a3221.

@Sannis Agreed, last commit puts each history entry on it's own line in the file. Good on call on supporting grep.

Owner

bnoordhuis commented Apr 30, 2012

@regality: Can you add some tests? Thanks.

@piscisaureus: Can you review this PR as well?

Sannis commented Jun 7, 2012

Is therу any chance that this will be merged in near future?

regality commented Jun 7, 2012

@Sannis, I chatted with @isaacs about it a while back and he said we could expect this to be pulled in for v0.9, so probably not too soon, but it's on the todo-list.

Sannis commented Jun 7, 2012

Nice!

isaacs commented Jun 9, 2012

@Sannis @regality Just to clarify:

I don't recall saying "We'll merge this patch." I believe I said something to the effect of, "we want some kind of nicer experience on the repl, and not having history or setup is kind of painful."

I have not looked into this patch in depth, and we'll have to be very careful about it. A lot of people use the repl, and it's also used by the debugger, and as a utility to administer many production servers.

v0.8 is imminent, probably going to land in the next week or so. We'll discuss the best route forward once 0.8 is forked and we're into 0.9. A discussion on the nodejs-dev mailing list would be worthwhile as well, just to get some input.

regality commented Jun 9, 2012

@isaacs Sorry about the misunderstanding, I guess I just got a bit too excited.

As for this patch, I can't see it breaking anything, it should be reverse compatible with any existing node setup.

I'll go ahead and start a thread on the mailing list and we can revisit things after v0.8 lands.

japj commented Jun 14, 2012

+1

@josher19 josher19 commented on an outdated diff Jun 15, 2012

lib/readline.js
@@ -503,6 +507,18 @@ Interface.prototype._line = function() {
};
+Interface.prototype.getHistory = function() {
+ return this.history;
+};
+
+Interface.prototype.setHistory = function(history) {
+ this.history = history;
+ this.historyIndex = -1;
+ if (this.history.length > kHistorySize) {
+ this.history.splice(kHistorySize, Infinity);
@josher19

josher19 Jun 15, 2012

Infinity not needed here.

Suggest replace whole function with:

Interface.prototype.setHistory = function(history) {
  this.historyIndex = -1;
  this.history = history.slice(0, kHistorySize);
}

so as to avoid side effects such as:

h = [1,2,3,4];
repl.setHistory(h);
h.unshift(0);
console.assert(repl.getHistory() === 1, "0 instead of 1"); // 0 instead of 1.

@teramako teramako commented on the diff Nov 7, 2012

lib/repl.js
@@ -352,6 +352,19 @@ REPLServer.prototype.resetContext = function(force) {
this.context = context;
};
+REPLServer.prototype.loadContext = function(file) {
+ var Module = require('module');
+ var context;
+ context = Module._load(file);
+ if (context && typeof context === 'object') {
+ for (var key in context) {
+ if (context.hasOwnProperty(key)) {
+ this.context[key] = context[key];
+ }
+ }
+ }
+};
@teramako

teramako Nov 7, 2012

ensure the file exists and set all exported properties

REPLServer.prototype.loadContext = function(file) {
  var self = this,
      Module = require('module');
  fs.exists(file, function(exists) {
    if (!exists) return;
    var context = Module._load(file);
    if (context && typeof context === 'object') {
      Object.getOwnPropertyNames(context).forEach(function(key) {
        Object.defineProperty(self.context, key, Object.getOwnPropertyDescriptor(context, key));
      });
    }
  });
};
@regality

regality Oct 3, 2013

the context file not being found is handled in src/node.js

coolaj86 commented Feb 8, 2013

+1

Can one of the admins verify this patch?

regality commented Oct 3, 2013

Just updated this to work with master and got all tests passing. What's the chance of getting this merged in before 0.12?

@regality have you run the tests for this patch? Seems failing on smartOS-x64, or at least it shows that.

regality commented Jan 2, 2014

I think the failing tests are unrelated tests that are also failing on master.

+1

Member

robertkowalski commented May 15, 2015

I think this is fixed in nodejs/io.js#1513

Owner

jasnell commented May 19, 2015

@robertkowalski ... should we backport or defer to the converged repo to pick it up?

Owner

Fishrock123 commented May 19, 2015

Hmmm, might be best to at least choose one or the other with the combined TSC if you want to land here.

Edit: this PR is pretty old.. I'd suggest using the io.js one.

Owner

jasnell commented May 19, 2015

+1 ... at this point, recommend just closing this and picking up the io.js change in the converged stream.

cjihrig commented May 19, 2015

+1 to what @jasnell said.

Member

robertkowalski commented May 19, 2015

+1 on closing

btw: if we would backport it it would be in node 0.13 right? and 0.13 does not really exist i think or did i miss somthing?

Owner

jasnell commented May 19, 2015

Yes, I'd assume v0.13. It's not yet entirely clear.
On May 19, 2015 1:30 PM, "Robert Kowalski" notifications@github.com wrote:

+1 on closing

btw: if we would backport it it would be in node 0.13 right? and 0.13 does
not really exist i think or did i miss somthing?


Reply to this email directly or view it on GitHub
joyent#3178 (comment).

Owner

jasnell commented May 19, 2015

Ok, closing. If we end up doing a backport we'll pick it up in a different PR.

@jasnell jasnell closed this May 19, 2015

I haven't stayed up to date on all the node.js/io.js politics, so I don't know if it would be better to keep this pull or not, plus it is has been neglected and become very old. But it is worth noting that nodejs/io.js#1513 requires you to set NODE_REPL_HISTORY instead of just looking in your home directory, which seems silly to me. This pull request also creates the .noderc.js functionality, which was not replicated in the other pull that handles node history.

In short, closing this may be the best course of action, but moving some of the changes to a new pull request might also be a good idea.

Owner

Fishrock123 commented May 19, 2015

Yeah, perhaps it's best to open a new issue on io.js discussing that?

Do note getting the home directory is currently flawed until libuv/libuv#350 lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment