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

fix(scaffold): config file is always generated at the project root #801

Merged
merged 6 commits into from Mar 26, 2019

Conversation

anshumanv
Copy link
Member

config file is generated at the project root

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?
yes

If relevant, did you update the documentation?
NA

Summary
Determine project root and create the file there.

Does this PR introduce a breaking change?
No

Other information
Ref - #709

config file is generated at the project root
@anshumanv
Copy link
Member Author

Looking into CI

Copy link
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.

Have a look at suggestion 😃

packages/utils/scaffold.ts Outdated Show resolved Hide resolved
@rishabh3112
Copy link
Member

Also, commit package-lock.json when you install or update packages :)

@anshumanv
Copy link
Member Author

Also, commit package-lock.json when you install or update packages :)

Sure thing, on it. 👍

add package-lock for utils as new dep was introduced
Copy link
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.

I'd suggest to not add another package. This package basically uses findup-sync, which is already installed in the core cli.

A nice refactoring would be to move this package inside the utility package and expose proper functions to whatever package wants to use it

packages/utils/scaffold.ts Outdated Show resolved Hide resolved
@webpack-bot
Copy link

@anshumanv Thanks for your update.

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

@ematipico Please review the new changes.

@webpack-bot
Copy link

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

@anshumanv
Copy link
Member Author

@ematipico this approach looks amazing ✨

Thanks for suggesting this, looks way more maintainable and cleaner. 👍

migrated to use find-up sync to determine project root, removed additional packages
Copy link
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.

Almost there! Few changes to do

packages/utils/find-root.ts Show resolved Hide resolved
packages/utils/find-root.ts Outdated Show resolved Hide resolved
packages/utils/scaffold.ts Outdated Show resolved Hide resolved
@anshumanv
Copy link
Member Author

@ematipico fixed the issues, can we go ahead with this once builds are green? Will start working on the follow-up PR soon. ✨

@dhruvdutt
Copy link
Member

Looks good to go. Nice work. 💯

@ematipico ematipico merged commit 3e870e2 into webpack:master Mar 26, 2019
@anshumanv anshumanv deleted the config-in-root branch March 27, 2019 08:33
misterdev pushed a commit to misterdev/webpack-cli that referenced this pull request Apr 23, 2019
…ebpack#801)

* fix(scaffold): config file is always generated at the project root

config file is generated at the project root

* chore(scaffold): add package-lock for utils

add package-lock for utils as new dep was introduced

* chore(utils): fix ci

fix ci

* fix(scaffold): migrated to findup-sync to find the project root

migrated to use find-up sync to determine project root, removed additional packages

* chore(utils): added findup-sync package to utils package

added findup-sync to utils package

* chore(scaffold): do not pass process.cwd()

do not pass process.cwd()
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.

None yet

6 participants