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

Command Mode not working on Windows #239

Closed
adrianjost opened this issue Mar 7, 2020 · 8 comments · Fixed by #244
Closed

Command Mode not working on Windows #239

adrianjost opened this issue Mar 7, 2020 · 8 comments · Fixed by #244
Labels

Comments

@adrianjost
Copy link
Contributor

Bug

  • version: 1.10.8
  • platform: windows

Description

The command mode does not work correctly on windows. The problem seems to be in the parsing of the CLI arguments. On Windows the arguments get split at every space and ignores any ' that should group arguments to a single command.

Example

command: npx start-test start:storybook 4001 'jest --config tests/screenshot/jest.config.js --ci'

actual behavior

compare the second line between Windows and Ubuntu

Windows Debug Output

> npx start-test start:storybook 4001 'jest --config tests/screenshot/jest.config.js --ci'
start-server-and-test parsing CLI arguments: [ 'start:storybook', '4001', "'jest", '--config', 'tests/screenshot/jest.config.js', "--ci'" ]

 throw err;
      ^

Error: expected <NPM script name that starts server> <url or port> <NPM script name that runs tests>
example: start-test start 8080 test
see https://github.com/bahmutov/start-server-and-test#use

    at lazyAssLogic (workdir\node_modules\lazy-ass\index.js:110:14)
    at lazyAss (workdir\node_modules\lazy-ass\index.js:115:28)
    at Object.getArguments (workdir\node_modules\start-server-and-test\src\utils.js:35:5)
    at Object.<anonymous> (workdir\node_modules\start-server-and-test\src\bin\start.js:10:22)
    at Module._compile (internal/modules/cjs/loader.js:1157:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1177:10)
    at Module.load (internal/modules/cjs/loader.js:1001:32)
    at Function.Module._load (internal/modules/cjs/loader.js:900:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
    at internal/main/run_main_module.js:18:47

Ubuntu Debug Output

> npx start-test start:storybook 4001 'jest --config tests/screenshot/jest.config.js --ci'
start-server-and-test parsing CLI arguments: [ 'start:storybook', '4001', 'jest --config tests/screenshot/jest.config.js --ci' ]
  start-server-and-test parsed args: { start: 'npm run start:storybook', url: [ 'http://localhost:4001' ], test: 'jest --config tests/screenshot/jest.config.js --ci' }

expected behaviour

  • execute jest with config in ci mode instead of throwing an error
  • parse commands like on Ubuntu and in the README
@adrianjost
Copy link
Contributor Author

Sorry for opening an issue to early. After investigating your source code I discovered that this is expected behaviour of node.js. You have to escape the quotes on windows to have it working.

So on Windows you have to execute:

  • npx start-test start:storybook 4001 \"jest --config tests/screenshot/jest.config.js --ci\"

and if you want to put this into a script in the package.json, you have to escape it even more:

{
  "scripts": {
    "test": "npx start-test start:storybook 4001 \\\"jest --config tests/screenshot/jest.config.js --ci\\\""
  }
}

generic command:

{
  "scripts": {
    "test": "npx \\\"server start command\\\" 4001 \\\"test command\\\""
  }
}

@adrianjost
Copy link
Contributor Author

adrianjost commented Mar 7, 2020

As I investigated further, I discovered that the working windows command does not work anymore on Ubuntu.

Maybe you could implement a fix that concats arguments if they begin with ' or " and merge them until an argument ends with ' or ". This would make it possible to run on both systems with the same command.

Implementation Idea:

let args = process.argv.slice(2)

let concatMode = false;
const combinedArgs = []
for (let i = 0; i < args.length; i++){
  let arg = args[i];
  if(args[i].startsWith(`'`) || args[i].startsWith(`"`)){
    arg = arg.slice(1)
  }
  if(arg.endsWith(`'`) || arg.endsWith(`"`)){
    arg = arg.slice(0,-1)
  }

  if(concatMode && combinedArgs.length){
    combinedArgs[combinedArgs.length - 1] += arg
  }else{
    combinedArgs.push(arg)
  }

  if(args[i].startsWith(`'`) || args[i].startsWith(`"`)){
    concatMode = true;
  }
  if(args[i].endsWith(`'`) || args[i].endsWith(`"`)){
    concatMode = false;
  }
}

// now use combinedArgs instead of args

@adrianjost adrianjost reopened this Mar 7, 2020
@bahmutov
Copy link
Owner

bahmutov commented Mar 7, 2020 via email

@johannesjo
Copy link

It would be really great if we could merge the work around. I am struggling with this as well...

@lone-cloud
Copy link

The fix for this has been sitting around for almost a year now. Does anyone still care?

@bahmutov
Copy link
Owner

bahmutov commented Feb 2, 2021

Can the author resolve the merge conflicts, please

@adrianjost
Copy link
Contributor Author

Sure, will do 👍

@github-actions
Copy link

🎉 This issue has been resolved in version 1.12.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants