feat(server): add stdin for API by knagaitsev · Pull Request #2106 · webpack/webpack-dev-server

knagaitsev · 2019-07-06T16:44:57Z

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

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 · 2019-07-06T17:00:10Z

 this.compiler = compiler;
 this.options = options;

+ handleStdin(options);


hm, why we need it here?

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

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

alexander-akait · 2019-07-06T17:46:09Z

+ });
+
+ process.stdin.resume();
+ }


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

@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

/cc @hiroppy

codecov · 2019-07-07T13:34:29Z

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 · 2019-07-07T17:47:09Z

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 · 2019-07-07T18:54:04Z

@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 · 2019-07-08T10:12:09Z

/cc @Loonride

knagaitsev · 2019-07-09T00:49:37Z

@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 · 2019-07-09T03:39:07Z

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 · 2019-07-09T04:31:09Z

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 · 2019-07-09T11:29:46Z

@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 · 2019-07-09T20:23:26Z

@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 · 2019-07-24T05:36:15Z

/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 · 2019-07-24T06:32:37Z

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

knagaitsev · 2019-08-10T04:03:53Z

Closing in favor of #2186

knagaitsev requested review from alexander-akait and hiroppy as code owners July 6, 2019 16:44

alexander-akait reviewed Jul 6, 2019
View reviewed changes

knagaitsev force-pushed the stdin-option branch from df71ad6 to 5c20c65Compare July 6, 2019 17:53

alexander-akait previously approved these changes Jul 6, 2019
View reviewed changes

alexander-akait closed this Jul 7, 2019

alexander-akait reopened this Jul 7, 2019

alexander-akait closed this Jul 8, 2019

alexander-akait reopened this Jul 8, 2019

knagaitsev dismissed alexander-akait’s stale review via 5ba2778July 9, 2019 03:37

knagaitsev force-pushed the stdin-option branch from 957bf86 to 5ba2778Compare July 9, 2019 03:37

knagaitsev closed this Jul 12, 2019

knagaitsev reopened this Jul 12, 2019

knagaitsev closed this Jul 12, 2019

knagaitsev reopened this Jul 12, 2019

knagaitsev closed this Jul 12, 2019

knagaitsev reopened this Jul 12, 2019

knagaitsev force-pushed the stdin-option branch from fc3cf48 to 1a1b1a7Compare July 12, 2019 16:49

knagaitsev closed this Jul 13, 2019

knagaitsev reopened this Jul 13, 2019

knagaitsev added 10 commits July 23, 2019 17:43

test(options): remove end emit
33fb577

test(server): remove some stdin end emits
ff5d730

test(server): add stdin pause next to end emit
5cbc2a4

test(cli): switch back to execa from spawn
d849568

test(cli): upgrade execa
d182b07

test(cli): switch back to spawn
17fddfc

test(cli): downgrade execa again
d670177

test(server): forcefully remove stdin end listeners
2bc9387

fix(package): fix package lock
0c9f4bb

fix(package): fix package lock again
ddc12d3

knagaitsev force-pushed the stdin-option branch from 4893850 to ddc12d3Compare July 23, 2019 22:45

test(server): skip new stdin tests
48a40b4

knagaitsev closed this Jul 24, 2019

knagaitsev reopened this Jul 24, 2019

knagaitsev added 10 commits July 23, 2019 20:45

test(server): removed stdin tests entirely
fcef89d

test(server): remove cli test
2634594

test(server): remove options test
5db844a

test(server): add back stdin files
b3b3299

test(server): only cli stdin
959c0ed

test(server): add handle stdin helper only
66cd1df

test(server): add stdin option test only
bf90582

test(server): only handle stdin
28a7073

test(server): move stdin pause after a delay following stdin resume
ec41502

test(options): removed exit mock
b35e8ac

knagaitsev mentioned this pull request Aug 10, 2019
feat(server): add stdin option to API #2186
Merged
6 tasks

knagaitsev closed this Aug 10, 2019

knagaitsev added the gsoc Google Summer of Code label Aug 13, 2019

Uh oh!

Uh oh!

Conversation

knagaitsev commented Jul 6, 2019• edited by alexander-akait Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

Uh oh!

alexander-akaitJul 6, 2019

Choose a reason for hiding this comment

Uh oh!

knagaitsevJul 6, 2019

Choose a reason for hiding this comment

Uh oh!

alexander-akaitJul 6, 2019

Choose a reason for hiding this comment

Uh oh!

alexander-akaitJul 6, 2019

Choose a reason for hiding this comment

Uh oh!

knagaitsevJul 6, 2019

Choose a reason for hiding this comment

Uh oh!

alexander-akaitJul 6, 2019

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

codecovBot commented Jul 7, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hiroppy commented Jul 7, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EslamHiko commented Jul 7, 2019

Uh oh!

alexander-akait commented Jul 8, 2019

Uh oh!

knagaitsev commented Jul 9, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knagaitsev commented Jul 9, 2019

Uh oh!

knagaitsev commented Jul 9, 2019

Uh oh!

alexander-akait commented Jul 9, 2019

Uh oh!

knagaitsev commented Jul 9, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knagaitsev commented Jul 24, 2019

Uh oh!

hiroppy commented Jul 24, 2019

Uh oh!

knagaitsev commented Aug 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

knagaitsev commented Jul 6, 2019•
edited by alexander-akait
Loading

codecovBot commented Jul 7, 2019•
edited
Loading

hiroppy commented Jul 7, 2019•
edited
Loading

knagaitsev commented Jul 9, 2019•
edited
Loading

knagaitsev commented Jul 9, 2019•
edited
Loading