search

abicky / nodejs-repl.el

Run Node.js REPL and communicate with the process
190 stars 38 forks source link

Introduce nodejs-repl-eval-script defcustom #11

Closed arichiardi closed 7 years ago

arichiardi commented 7 years ago

This allows users to prepend, replace or event disable the automatic execution of -e ... at the process command line. The option should be a cons cell where the car is a the Javascript string to evaluate and the cdr is the behavior nodejs-repl should apply while bootstrapping, e.g.:

By default, the number of columns of the process is set and the repl is started. Note that:

Completely avoids passing the eval script to the Node.js process.

abicky commented 7 years ago

Thank you for your pull request! As you mention in https://github.com/abicky/nodejs-repl.el/issues/10#issuecomment-301114696, I think the issue should be resolved by @types/node.

% ts-node --compilerOptions '{"types":["node"]}'
> process.stdout.columns
⨯ Unable to compile TypeScript
[eval].ts (1,16): Property 'columns' does not exist on type 'Socket'. (2339)
> process.stdout['columns']
88

This PR might help you to launch ts-node in Emacs, but I think nodejs-repl won't work correctly. For example, Emacs hung when I tried token completion without -e option.

arichiardi commented 7 years ago

It works perfectly with ts-node and it is optional. Completion works super smoothly. Did you try with ts-node?

arichiardi commented 7 years ago

I will try better on my side as well, please tell me the test that hangs so that I can try.

abicky commented 7 years ago

I found that emacs would hang if nodejs-repl-prompt was changed as below:

(setq nodejs-repl-prompt "node> "
      nodejs-repl-command "ts-node"
      nodejs-repl-bootstrap-repl nil)

It is because this line won't be true.

abicky commented 7 years ago

I think -e option might not be required anymore, because I can't reproduce this comment. I don't remember the combination of the emacs version and the nodejs version to reproduce it...

arichiardi commented 7 years ago

Uhm so you found actually a related bug because you changed the repl prompt..

I did not change the prompt here.

Do you think it might be worth dropping the logic given the old node version (0.8)? I can do that.

arichiardi commented 7 years ago

@abicky so what do you suggest I do in order to improve this PR?

arichiardi commented 7 years ago

Ok I pushed a version that hopefully makes things better.

arichiardi commented 7 years ago

@abicky The tests are failing because i am using a couple of functions for Emacs 24.4, are you planning to support old Emacs or you are ok with dropping?

Related: http://emacsredux.com/blog/2013/09/05/version-checks/

abicky commented 7 years ago

CI failed 😢 https://travis-ci.org/abicky/nodejs-repl.el/builds/233310786?utm_source=github_status&utm_medium=notification

I could reproduce the comment in Node.js v0.8.28 and Node.js v0.10.41 (not in Node.js v4.x). The comment mentions the implementation of Interface#columns which depends on process.stdout.columns. This result will be Infinity if process.stdout.columns, which comes from winsize.ws_col, is 0. Therefore the value should be set.

Node.js v0.12.18 or earlier is not supported officially but I want to support them rather than remove -e option. On the other hand, I don't think your change is useful relative to its complexity.

I suggest you create a pull request in DefinitelyTyped/DefinitelyTyped to support process.stdout.columns and write the following code in your init.el temporarily:

(setq nodejs-repl-code-format
  (concat
   "//process.stdout.columns = %d;"
   "require('repl').start('%s', null, null, true, false, "
   "require('repl')['REPL_MODE_' + '%s'.toUpperCase()])"))
arichiardi commented 7 years ago

I disagree here a bit, the eval code is artificial and could cause other side effects that people wouldn't want to have. My path allows exactly to make the bootstrap code malleable:

About CI, I wrote above why is failing. It is unrelated to what you reported above. If you are ok with patch I can use functions compatible with old Emacs versions. The support for now is on > 24.5 only but I can change that.

abicky commented 7 years ago

Umm, I don't think the code causes side effects. It just sets process.stdout.columns properly and process.stdout.columns usually exists. In addition, I think nodejs-repl.el users shouldn't be conscious of how it runs Node.js REPL.

arichiardi commented 7 years ago

Not sure I agree either, things should be configurable and disablable. People might be surprised when they don't see the same behavior when launching in the terminal..In any case...On the other hand mybe you are right, if nobody asked this feature maybe folks don't want to be able to customize the -e option at all. Still think hardcoded stuff is not ideal but hey, maybe it is just me 😀 So feel free to close this, I will keep it in my fork only. Thanks!

abicky commented 7 years ago

Definitely, it is not a good way to use -e option to launch REPL but it is necessary for old Node.js. I think it is better to stop supporting Node.js 0.12.18 or earlier or to change how to launch REPL by Node.js versions automatically, not to introduce custom variables, so I will close this PR. Thank you for your contribution!