getSettings throws error with paths with spaces in Windows

[rossknudsen]

When passing a workspace to getSettings function throws error if paths contain spaces.

Repro:

// test-jest-editor-support.js

const support = require("jest-editor-support");
const {ProjectWorkspace, getSettings} =support;

const projectWorkspace = new ProjectWorkspace(
    "C:\\Users\\rossk\\Desktop\\cra test",
    "C:\\Users\\rossk\\Desktop\\cra test\\node_modules\\.bin\\jest.cmd",
    "C:\\Users\\rossk\\Desktop\\cra test\\jest.config.js",
    20,
    undefined,
    false,
    false,
  );

getSettings(projectWorkspace).then(results => console.log(results)).catch(error => console.error(error));

[seanpoulter]

Thanks for bringing the issue over Ross. It is definitely a problem. We've had problems in the past handling filenames so I'm not surprised to see this come up again. Have you had look at it at all? It looks like getSettings is calling createProcess which ends up calling spawn with everything as the command and an empty args array.

My two cents is that we should really be using those args. If I remember correctly, that would bubble up to making a change to the user settings in vscode-jest. 🤷

Here's the callsite for spawn(...) - https://github.com/jest-community/jest-editor-support/blob/master/src/Process.js#L49 Here's the docs for spawn - https://nodejs.org/api/child_process.html#child_process_child_process_spawn_command_args_options

[rossknudsen]

Hi Sean, no I haven't looked at it until now, I've been responding to issues in the vscode extension that uses this library. But since you've done all the leg work, I'll give it a go. Hopefully its about as simple as putting quotes around the path.

[rossknudsen]

Hmmm, I see why this caused headaches in the past...

I have a couple of solutions but I need to discuss them before pursuing further:

1st: pass in the args as received and put double quotes around the executable

// ...
const command = `"${workspace.pathToJest}"`;
// ...
return spawn(command, args, spawnOptions);

The unit tests need to be fixed up, but the test data uses npm test -- which then gets quoted. But when quoted, the command wouldn't work on the command line. So I could try some logic to figure out whether the given command is a file path or not, but I suspect that will be fragile.

2nd: pass args in as received but following the advice from this SO answer, create a command (the name of the executable) and then set the cwd in the spawn options.

// ...
const {base: command, dir} = path.parse(workspace.pathToJest);
// ...
const spawnOptions = {
  cwd: dir
};
// ...
return spawn(command, args, spawnOptions);

This seems to work, but will there be issues with changing the working directory from workspace.rootPath? Maybe there are other args that are relative file paths?

[rossknudsen]

I guess we need to decide what are valid values for workspace.pathToJest. I think the following are all reasonable, basically we should be expecting a path - or the special case of a globally installed jest:

• "jest"
• "c:\source\project with spaces\node_modules.bin\jest.cmd"
• "/home/rossk/source/project with spaces/node_modules/.bin/jest"

I don't think commands are reasonable, such as what is currently in the unit tests:

• "npm test --"

In that case we are mixing commands with args and its going to be messy.

[seanpoulter]

Thanks Ross. It gets messy, doesn't it? I remember having quite a bit of conversation about how we can be clever to handle it. The path.parse is neat.

I think parsing a string into a command, arguments, and a directory across multiple platforms is an interesting problem, but personally I don't think we should try to solve it. It's not to say we can't, but I'd rather steer folks towards the Jest-related features like all the work going in your MRs.

If we have problems because we're using pathToJest as a string instead of the command, args, and cwd for spawn why don't we give folks the options to configure that?

type Command = /* Ideas .. Process, Command, CommandLine, Executable, SpawnOptions ... */
  string | {command: string, args: string[], dir?: string}

[rossknudsen]

I like the way you're thinking, I have to remember this is a low-level support library and so we can leave some of that hardwork up to the consumer. I did realize though that the function that I'm focusing on (createProcess) is an internal function so we might have to change the shape of the ProjectWorkspace type which is in the public API. Based on what you've described above, here is what I suggest for a change to that API. Let me know your thoughts, since it will be a breaking change.

 export default class ProjectWorkspace {
   /**
    * The path to the root of the project's workspace
    *
    * @type {string}
    */
   rootPath: string;

-  /**
-   * The path to Jest, this is normally a file path like
-   * `node_modules/.bin/jest` but you should not make the assumption that
-   * it is always a direct file path, as in a create-react app it would look
-   * like `npm test --`.
-   *
-   * This means when launching a process, you will need to split on the first
-   * space, and then move any other args into the args of the process.
-   *
-   * @type {string}
-   */
-  pathToJest: string;
+  /**
+   * The command to run Jest.
+   *
+   * E.g. 'jest' => invoke Jest globally, or in the working directory.
+   * E.g. 'npm run test' => invoke a shell commmand that executes the Jest process.
+   */
+  jestCommand: string;
+
+  /**
+   * Any arguments that should be passed along with the jest command.  Please escape any
+   * characters, such as whitespace, appropriately for command line execution.
+   */
+  jestArgs?: string[];
+
+  /**
+   * Optionally the working directory where the Jest process should be executed.  Will
+   * default to the workspace root if not supplied.
+   */
+  workingDirectory?: string;

[connectdotz]

If we are to stick with spawning process with shell, there is not much advantage to ask our client to separate the command from args. We could just rename the ProjectWorkspace.pathToJest to ProjectWorkspace.jestCommandline, and let the callers pass in whatever command-line they want and the quotes, escape characters etc will all be preserved without us doing any complicated maneuver here.

We should make the same change in vscode-jest, we have already used the pathToJest as the full commandline for a very long time, just never did bring ourselves to rename it accordingly... now with a major release 4.0 in the plan, we could consider schedule this in.

[rossknudsen]

Overnight, I thought of multiple issues with the approach I had been working on. I think you're right @connectdotz, we have to leave it to the consumers to escape paths correctly. I think I jumped the gun initially with my script to expose the issue. I'll close this issue now.