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

Refactor npmPackagesExists() function #622

Merged
merged 1 commit into from Oct 30, 2018
Merged

Conversation

styfle
Copy link
Contributor

@styfle styfle commented Sep 30, 2018

What kind of change does this PR introduce?
Refactoring npmPackagesExists() function

Did you add tests for your changes?
No, there is no API change

If relevant, did you update the documentation?
No

Summary

This makes the code more readable and avoids checking the boolean twice

Does this PR introduce a breaking change?
No

Other information

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

This might have pitfalls, cause maybe we try to resolve the local one later. Putting this on chill for a bit

@webpack-bot
Copy link

@styfle Thanks for your update.

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

@ematipico Please review the new changes.

@ematipico
Copy link
Contributor

@ev1stensberg could you explain? I don't see how: an error is always thrown

@evenstensberg
Copy link
Member

The initial idea is to run this on multiple packages, so you can combine different scaffolds. I don't know if it still works ( might need some tweaking ).

How is this handled now? How is the error thrown? I think we got a setup for it. We need to look at potential errors this might raise.

@webpack-bot
Copy link

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

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

lgtm for now, as we only support one package not array of packages to scaffold. We should change the API to support arrays better in the future

@dhruvdutt dhruvdutt merged commit 3e3c94d into webpack:master Oct 30, 2018
@styfle styfle deleted the patch-1 branch October 30, 2018 18:29
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

5 participants