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

feat: cli options #3325

Merged
merged 14 commits into from
Jun 8, 2021
Merged

feat: cli options #3325

merged 14 commits into from
Jun 8, 2021

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented May 21, 2021

  • 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

#3300 (comment)

Breaking Changes

No

Additional Info

No

@codecov
Copy link

codecov bot commented May 22, 2021

Codecov Report

Merging #3325 (2edd362) into master (cef3d85) will decrease coverage by 0.46%.
The diff coverage is 61.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3325      +/-   ##
==========================================
- Coverage   95.72%   95.25%   -0.47%     
==========================================
  Files          33       34       +1     
  Lines        1309     1327      +18     
  Branches      388      390       +2     
==========================================
+ Hits         1253     1264      +11     
- Misses         52       59       +7     
  Partials        4        4              
Impacted Files Coverage Δ
bin/cli-flags.js 100.00% <ø> (ø)
lib/Server.js 95.01% <61.11%> (-1.27%) ⬇️

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 84bd080...2edd362. Read the comment docs.

@alexander-akait
Copy link
Member

@snitin315 I think better finish this before merge it #3309 (here we finish our options)

@snitin315
Copy link
Member Author

It should be good now. Waiting for green CI.

@snitin315
Copy link
Member Author

I have not included -reset flags, I think we need webpack.processArguments for their proper parsing.

@alexander-akait
Copy link
Member

I think we need webpack.processArguments for their proper parsing.

Ideally - yes, but I think we need -reset flags here, otherwise we will need to do new major release in future, --reset should set array on [], so it should be easy to implement

In theory we can move part of logic webpack.processArguments in own code

@snitin315
Copy link
Member Author

I will update 👍

@snitin315 snitin315 force-pushed the new-cli branch 2 times, most recently from 5b4e85e to d2acaba Compare May 23, 2021 11:41
@snitin315
Copy link
Member Author

Done ✅

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, let's merge it after #3309

@snitin315
Copy link
Member Author

MacOS issue, help output is short in CI.

@@ -586,6 +862,14 @@ describe('CLI', () => {
});

it('should generate correct cli flags', (done) => {
const isMacOS = process.platform === 'darwin';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is problem? Not all output? It means we have bug somewhere...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think problem with exec nodejs/node#19218

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch 👍

})
.catch(done);
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add test: using webpack server --config ./path/webpack.config.js with --watch-files-paths-reset --watch-files-paths ${watchDirectory} and do snapshot (you can set stats: false, we will improve it late), as you can see using expect(output.exitCode).toEqual(0); is bad testing, we need snapshot stdout and stderr, it allow use to see what we rewrite watch-files-paths

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't log which files are being watched on stderr/stdout

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though we do log for static, added more snapshots 👍🏻

@snitin315 snitin315 force-pushed the new-cli branch 3 times, most recently from 518685d to 34c46b4 Compare May 28, 2021 09:50
@snitin315
Copy link
Member Author

I will rebase in near future.

@alexander-akait
Copy link
Member

@snitin315 I will finish it, WIP, also I will add test to ensure we don't lose cli args in future after adding/removing/changes our options

@snitin315
Copy link
Member Author

CI is green ✅

@alexander-akait alexander-akait merged commit 70ed7dc into master Jun 8, 2021
@alexander-akait alexander-akait deleted the new-cli branch June 8, 2021 14:10
@snitin315 snitin315 mentioned this pull request Jun 9, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants