-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Cleaning up printHostingInstructions a bit #3036
Conversation
It would be nice to merge this, if possible. |
Hello! I'm a bot that helps facilitate testing pull requests. Your pull request (commit c67617b) has been released on npm for testing purposes. npm i react-scripts-dangerous@1.0.13-c67617b.0
# or
yarn add react-scripts-dangerous@1.0.13-c67617b.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.13-c67617b.0 folder/ Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk! Thanks for your contribution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide screenshots verifying you've tested all code paths this could take?
const publicPathname = url.parse(publicPath).pathname; | ||
const hasDeployScript = typeof appPackage.scripts.deploy !== 'undefined'; | ||
printBaseMessage(buildFolder, publicPathname); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whitespace can be removed (and below too)
Is anything else needed for this PR? |
I know this is pretty low priority, but I'd like to see it merged before it becomes out of date. |
Sorry, we've been focused on getting React 16 out and didn't have enough time to review. |
Thanks! |
Understandable, thanks for the merge! |
* Replacing literal 'build' with `buildFolder` variable * Cleaning up the printHostingInstructions a bit * Fixing undefined variable
There are no functional or text changes with this PR. I'm just cleaning up the logic for printHostingInstructions to make it more readable and remove some duplication. The only minor logic change is that I replaced a couple hard-coded literals (
'build'
) with variables (buildFolder
).The main
printHostingInstructions
method is now purely logic. It calls three other functionsprintBaseMessage
,printDeployInstructions
, andprintStaticServerInstructions
; which contain the console statements with minimal logic.