Improve Windows compatibility #5

TheBrenny · 2020-10-06T10:20:14Z

Added a method to acquire the session history on windows. It doesn't get the history, and also doesn't check to see if the user has a HISTFILE environment variable. But I don't know if any windows user actually has one for CMD.

This won't work if invoked through PowerShell though, because PS uses the history command.

🙏

sindresorhus · 2020-10-06T11:45:39Z

I don't really understand the point. It gets only the session history, but not permanent history? However, the session history is nothing as we're spawning the terminal. Can you please elaborate?

TheBrenny · 2020-10-06T14:20:38Z

I know it doesn't get a persistent history, but getting the session's history is also a common use case (I imagine). Otherwise, if a HISTFILE env variable is set, that file should be used - because there are tools out there which persist CMD history across sessions.

Otherwise, a session history use case would be:

$ git clone https://github.com/sindresorhus/shell-history.git && alert

Where alert is an alias to node C:\\alert.js (for example), alert.js can read the session's history to make a toast notification with "git completed!" as the title.

I hope that makes more sense.

sindresorhus · 2020-10-09T00:05:44Z

but getting the session's history is also a common use case (I imagine).

But does it actually get the history of the whole session? Like commands executed before shell-history was running.

index.js

sindresorhus · 2020-10-09T00:09:14Z

Please always run npm test locally before committing.

sindresorhus · 2020-10-09T00:09:42Z

The special Windows behavior (of only getting the session history) needs to be documented.

TheBrenny · 2020-10-13T01:19:50Z

Please always run npm test locally before committing.

I tried running the tests but it just isn't working. First, there are issues because eslint-plugin-ava causes an undefined TypeError to throw, because that's how it's made. I did a clean install on WSL (which defeats the purpose of the test) which failed anyway because of the previous issue.

I then decided to skip the whole xo process (because none of Node12(Win), Node12(Linux) or Node14(Linux)) worked.

Calling node_modules/.bin/ava (both the sh and cmd variants on Win and WSL) didn't work because test.js uses ES6 syntax, which I'm not compatible with apparently...

At the end of the day, I converted test.js to Node's require syntax, called node_modules/.bin/ava and magic almost happened - I gave the test a name, and then it worked with 1 test passed.

I didn't plan on giving you my life story on trying to run XO and Ava, so sorry about that! 😅

The special Windows behavior (of only getting the session history) needs to be documented.

Doneskies.

Send feedback! 🎉

sindresorhus · 2020-10-13T20:47:15Z

I then decided to skip the whole xo process (because none of Node12(Win), Node12(Linux) or Node14(Linux)) worked.

XO works fine on all those setups. This is a problem with your setup. But you can look at the lint failures in the Travis output and fix manually.

index.js

sindresorhus · 2020-10-13T20:48:39Z

#5 (comment) is not resolved. Please read my comment again. I think it's quite clear.

TheBrenny · 2020-10-13T23:24:19Z

but getting the session's history is also a common use case (I imagine).

But does it actually get the history of the whole session? Like commands executed before shell-history was running.

Sorry I missed this comment -- I've added test_win.bat which shows how this works. I don't know how to turn that into a "test" per se, but it's a visual tester I guess... The only bug which I know of for this is when using clink (which overrides doskey, so it's a problem on that end, not here).

#6 Fixed my problems (big ty @rocktimsaikia 🎉) -- Test now passes correctly.

#5 (comment) is not resolved. Please read my comment again. I think it's quite clear.

You are correct it wasn't resolved - I didn't realise that spawnSync had an encoding option. (Off-topic, but what's the benefit over toString if its default behaviour is to encode to utf8?)

This has now been resolved.

sindresorhus · 2020-10-17T15:10:05Z

readme.md


 ### shellHistory.path()

 Get the path of the file containing the shell history.

+On Windows, this will return either the `HISTFILE` environment variable, or null.


I don't see any null in the code.

sindresorhus · 2020-10-17T15:10:39Z

index.js

@@ -63,7 +60,17 @@ module.exports = options => {
 	options = options || {};

 	if (process.platform === 'win32') {
-		return [];
+		const histFile = getPath(options);


The naming is an implementation detail of the getPath method.

Suggested change

const histFile = getPath(options);

const historyPath = getPath(options);

sindresorhus · 2020-10-17T15:11:06Z

index.js

+
+		// No histfile used -- spawn a doskey
+		const childProcess = require('child_process');
+		const doskey = childProcess.spawnSync('doskey', ['/history'], {


Suggested change

const doskey = childProcess.spawnSync('doskey', ['/history'], {

const {stdout} = childProcess.spawnSync('doskey', ['/history'], {

sindresorhus · 2020-10-17T15:11:24Z

index.js

+		const doskey = childProcess.spawnSync('doskey', ['/history'], {
+			encoding: 'utf8'
+		});
+		return doskey.stdout.trim().split('\r\n') || []; // Enforce array


The comment is moot. The code is clear enough.

sindresorhus · 2020-10-17T15:11:37Z

index.js

+		}
+
+		// No histfile used -- spawn a doskey
+		const childProcess = require('child_process');


Imports should be at the top of the file.

sindresorhus · 2020-10-17T15:12:02Z

test_win.bat

+@echo off
+
+echo Print entire history
+node -p "const history = require('./'); history()"


Suggested change

node -p "const history = require('./'); history()"

node -p "const history = require('.'); history()"

sindresorhus · 2020-10-17T15:12:24Z

test_win.bat

+pause
+echo.
+echo Print last command (which will be "test_win.bat")
+node -p "const history = require('./'); let h = history(); h[h.length-1]"


Suggested change

node -p "const history = require('./'); let h = history(); h[h.length-1]"

node -p "const history = require('.'); let h = history(); h[h.length - 1]"

sindresorhus · 2020-10-17T15:12:52Z

test_win.bat

@@ -0,0 +1,12 @@
+:: Acts as a windows tester.


This sentence is not clear enough and it's Windows.

sindresorhus · 2020-10-17T15:13:23Z

index.js

-		return '';
-	}
-
+	// HISTFILE might be cross-platform, so win32 check is after!


"might" is not a good explanation. Explain exactly when it would be.

sindresorhus · 2020-10-17T15:14:04Z

I don't know how to turn that into a "test" per se, but it's a visual tester I guess...

Then that should be explained.

sindresorhus · 2020-10-17T15:15:25Z

Off-topic, but what's the benefit over toString if its default behaviour is to encode to utf8?)

No technical benefit in this case. But it's more readable to declare it upfront. It does matter when it comes to streams though, as it might end up splitting Unicode characters incorrectly.

sindresorhus · 2020-10-17T15:16:04Z

Please spend some more time perfecting this pull request.

.gitignore

readme.md

index.js

test_win.bat

sindresorhus · 2021-03-07T10:58:44Z

Bump

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>

sindresorhus requested changes Oct 9, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

sindresorhus changed the title ~~Use doskey on windows platforms~~ Improve Windows compatibility Oct 9, 2020

rocktimsaikia mentioned this pull request Oct 13, 2020

Require Node.js 10 #6

Merged

sindresorhus reviewed Oct 13, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

TheBrenny and others added 3 commits October 14, 2020 09:24

added windows method

1b44983

fleshed out how windows behaviour works

66b61e6

reflected requests by owner

1a69ff3

TheBrenny force-pushed the master branch from 82249ca to 1a69ff3 Compare October 13, 2020 23:23

sindresorhus requested changes Oct 17, 2020

View reviewed changes

sindresorhus and others added 2 commits October 17, 2020 17:16

Update readme.md

39affc1

Updated for @sindresorhus's liking

70b08f1

TheBrenny force-pushed the master branch from 39affc1 to 70b08f1 Compare October 26, 2020 21:49

TheBrenny and others added 2 commits November 17, 2020 18:53

typo and description

4220393

Update index.js

9845883

sindresorhus requested changes Dec 20, 2020

View reviewed changes

Base automatically changed from master to main January 23, 2021 18:46

Apply suggestions from code review

82fabcf

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>

resolve code reviews

e4e2c0d

TheBrenny force-pushed the master branch from ba510a2 to e4e2c0d Compare March 9, 2021 10:26

sindresorhus merged commit 5258f2d into sindresorhus:main Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Windows compatibility #5

Improve Windows compatibility #5

TheBrenny commented Oct 6, 2020

sindresorhus commented Oct 6, 2020

TheBrenny commented Oct 6, 2020

sindresorhus commented Oct 9, 2020

sindresorhus commented Oct 9, 2020

sindresorhus commented Oct 9, 2020

TheBrenny commented Oct 13, 2020

sindresorhus commented Oct 13, 2020

sindresorhus commented Oct 13, 2020

TheBrenny commented Oct 13, 2020

sindresorhus Oct 17, 2020

sindresorhus Oct 17, 2020

sindresorhus Oct 17, 2020

sindresorhus Oct 17, 2020

sindresorhus Oct 17, 2020

sindresorhus Oct 17, 2020

sindresorhus Oct 17, 2020

sindresorhus Oct 17, 2020

sindresorhus Oct 17, 2020

sindresorhus commented Oct 17, 2020

sindresorhus commented Oct 17, 2020

sindresorhus commented Oct 17, 2020

sindresorhus commented Mar 7, 2021

	const histFile = getPath(options);
	const historyPath = getPath(options);

	const doskey = childProcess.spawnSync('doskey', ['/history'], {
	const {stdout} = childProcess.spawnSync('doskey', ['/history'], {

	node -p "const history = require('./'); history()"
	node -p "const history = require('.'); history()"

	node -p "const history = require('./'); let h = history(); h[h.length-1]"
	node -p "const history = require('.'); let h = history(); h[h.length - 1]"

Improve Windows compatibility #5

Improve Windows compatibility #5

Conversation

TheBrenny commented Oct 6, 2020

sindresorhus commented Oct 6, 2020

TheBrenny commented Oct 6, 2020

sindresorhus commented Oct 9, 2020

sindresorhus commented Oct 9, 2020

sindresorhus commented Oct 9, 2020

TheBrenny commented Oct 13, 2020

sindresorhus commented Oct 13, 2020

sindresorhus commented Oct 13, 2020

TheBrenny commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Oct 17, 2020

sindresorhus commented Oct 17, 2020

sindresorhus commented Oct 17, 2020

sindresorhus commented Mar 7, 2021