Skip to content

chore(lib): refactored lib utils and groups#1140

Merged
ematipico merged 1 commit intowebpack:nextfrom
pranshuchittora:refactor-cli-lib-grp-utils
Feb 10, 2020
Merged

chore(lib): refactored lib utils and groups#1140
ematipico merged 1 commit intowebpack:nextfrom
pranshuchittora:refactor-cli-lib-grp-utils

Conversation

@pranshuchittora
Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?
Refactoring
Did you add tests for your changes?
No
If relevant, did you update the documentation?
No
Summary

  • Object destructing to prevent deep level object refs (quite confusing and tedious to write)
  • repeated else if to switch

Does this PR introduce a breaking change?
No

Other information

Copy link
Copy Markdown
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

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

@pranshuchittora most changes looks good to me, I had a issue so left it below :)

Copy link
Copy Markdown
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Could you please rebase and address my few comments? Than you!

@webpack-bot
Copy link
Copy Markdown

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

Copy link
Copy Markdown
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

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

A small change rest looks good to me.

if ((process.env.NODE_ENV && process.env.NODE_ENV === 'production') || process.env.NODE_ENV === 'development') {
return process.env.NODE_ENV;
const NODE_ENV = process.env.NODE_ENV;
if ((NODE_ENV && NODE_ENV === 'production') || NODE_ENV === 'development') {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if ((NODE_ENV && NODE_ENV === 'production') || NODE_ENV === 'development') {
if (NODE_ENV && (NODE_ENV === 'production' || NODE_ENV === 'development')) {

anshumanv
anshumanv previously approved these changes Feb 9, 2020
Copy link
Copy Markdown
Member

@anshumanv anshumanv left a comment

Choose a reason for hiding this comment

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

Looks like a good refactor.

@webpack-bot
Copy link
Copy Markdown

@pranshuchittora Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@jamesgeorge007 Please review the new changes.

chore: fixed the changes requested in the PR review

chore: fixes from PR review
@pranshuchittora pranshuchittora force-pushed the refactor-cli-lib-grp-utils branch from 5c501e5 to afd37d4 Compare February 10, 2020 09:11
@ematipico
Copy link
Copy Markdown
Contributor

Thank you!

@ematipico ematipico merged commit 237887b into webpack:next Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants