feat(server): add stdin for API

[knagaitsev]

• This is a bugfix
• This is a feature
• This is a code refactor
• This is a test update
• This is a docs update
• This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

fixes #1712

This makes stdin an option for both CLI and API, and helps in moving configuration changes out of the CLI for CLI refactor.

Breaking Changes

None

Additional Info

[alexander-akait]

hm, why we need it here?

[knagaitsev]

@evilebottnawi Do you think no helper would be better? I just thought having helper would be cleaner

[alexander-akait]

i think we need this inside server file, it is logic for CLI only

[alexander-akait]

No, i don't mean we don't need help, i just answer why we use this here?

[knagaitsev]

@evilebottnawi The goal is to make this option not CLI-only. As mentioned in #1712 it is desired for this to be possible.

Also this is beneficial because I want to reduce what the CLI does as much as possible for the refactor.

[alexander-akait]

👍

[alexander-akait]

/cc @hiroppy

[codecov]

Codecov Report

Merging #2106 into master will increase coverage by 0.19%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #2106      +/-   ##
==========================================
+ Coverage   92.56%   92.76%   +0.19%     
==========================================
  Files          33       34       +1     
  Lines        1265     1271       +6     
  Branches      361      362       +1     
==========================================
+ Hits         1171     1179       +8     
+ Misses         87       85       -2     
  Partials        7        7

Impacted Files	Coverage Δ
lib/utils/createConfig.js	92.79% <0%> (+1.64%)	⬆️
lib/Server.js	97.35% <100%> (+0.01%)	⬆️
lib/utils/handleStdin.js	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b79f523...b35e8ac. Read the comment docs.

[hiroppy]

I think tests on Mac will be broken by these changes.

FAIL test/cli/cli.test.js (102.875s)
  ● CLI › --stdin without stdin end should time out

    Timeout - Async callback was not invoked within the 60000ms timeout specified by jest.setTimeout.Error: 

      119 |   });
      120 | 
    > 121 |   it('--stdin without stdin end should time out', (done) => {
          |   ^
      122 |     const configPath = resolve(
      123 |       __dirname,
      124 |       '../fixtures/simple-config/webpack.config.js'

      at Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
      at Suite.it (test/cli/cli.test.js:121:3)
      at Object.describe (test/cli/cli.test.js:17:1)

[EslamHiko]

@Loonride since you are testing after calling the helper, that ensures that process.stdin.resume() has been called I think you can check for this flag process.stdin._readableState.paused it's false when stdin is enabled. instead of waiting for it to time out.

[alexander-akait]

/cc @Loonride

[knagaitsev]

@EslamHiko I'm not seeing the process.stdin._readableState.paused on Windows 7, Node v8.10.0, so I'm not sure if that is the right flag to look at. But good suggestion, I will see if something else from _readableState can be used to check this

Edit: also it would time out without stdin option so probably not a good test.

[knagaitsev]

I stuck with a solution of waiting for a timeout, because working with child process stdin seemed unreliable when I tried to use stdin.isPaused().

[knagaitsev]

Oddly inconsistent behavior with execa where timedOut is undefined on MacOS. Seeing if checking for failed property will work on both. Possibly an execa issue?

[alexander-akait]

@Loonride looks like problem on execa side? Can you try next version of execa, maybe they fixed this, if it is works we can minimum required nodejs version for devDependcies to 8.9 and describe this in Contributing guide

[knagaitsev]

@Loonride looks like problem on execa side? Can you try next version of execa, maybe they fixed this, if it is works we can minimum required nodejs version for devDependcies to 8.9 and describe this in Contributing guide

@evilebottnawi why node 8.9? It looks like tests are passing in node 6, and the upgrade seems to have solved it.

Edit: or possibly not, some MacOS still not passing

[knagaitsev]

/cc @hiroppy @evilebottnawi I finally resolved the hanging test issues. I found that on some OS/node versions process.stdin.resume() and process.stdin.pause() behave differently, which made testing tough. The squash before merge would clean up this messy history, correct?

[hiroppy]

@Loonride Thanks. Yep, so don't worry.

[knagaitsev]

Closing in favor of #2186