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

Cleaning up printHostingInstructions a bit #3036

Merged
merged 3 commits into from
Jan 9, 2018

Conversation

GreenGremlin
Copy link
Contributor

@GreenGremlin GreenGremlin commented Aug 31, 2017

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 functions printBaseMessage, printDeployInstructions, and printStaticServerInstructions; which contain the console statements with minimal logic.

@GreenGremlin
Copy link
Contributor Author

It would be nice to merge this, if possible.
:)

@react-scripts-dangerous

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 [email protected]
# or
yarn add [email protected]
# or
create-react-app [email protected] 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!

Copy link
Contributor

@gaearon gaearon left a 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);

Copy link
Contributor

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)

@GreenGremlin
Copy link
Contributor Author

I'm fairly certain I covered them all here:

image
image

@GreenGremlin
Copy link
Contributor Author

One more

image

@GreenGremlin
Copy link
Contributor Author

And another

image

@GreenGremlin
Copy link
Contributor Author

And without build scripts

image

image

@GreenGremlin
Copy link
Contributor Author

image

@GreenGremlin
Copy link
Contributor Author

Is anything else needed for this PR?

@GreenGremlin
Copy link
Contributor Author

I know this is pretty low priority, but I'd like to see it merged before it becomes out of date.

@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

Sorry, we've been focused on getting React 16 out and didn't have enough time to review.

@gaearon gaearon merged commit 72b6eb8 into facebook:master Jan 9, 2018
@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

Thanks!

@gaearon gaearon added this to the 1.0.18 milestone Jan 9, 2018
@GreenGremlin
Copy link
Contributor Author

Understandable, thanks for the merge!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants