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

Don't assume the project is hosted at the root #94

Merged
merged 5 commits into from
Jul 24, 2016
Merged

Don't assume the project is hosted at the root #94

merged 5 commits into from
Jul 24, 2016

Conversation

dhruska
Copy link
Contributor

@dhruska dhruska commented Jul 22, 2016

  • Require package.json in webpack.config.prod.js and use homepage if set; otherwise use '/'

Fixes #21

* Require package.json in webpack.config.prod.js and use homepage if set; otherwise use '/'
@ghost
Copy link

ghost commented Jul 22, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

@sotojuan Looks like @dhruska beat you to it—did you have a similar implementation in mind? Are there any edge cases you can see here?

@dhruska Thanks for contributing! I’ll review next week, I need to get some sleep after releasing. 😄 Next time I’d appreciate if you coordinated on the issue first because it’s frustrating when somebody already starts their work, and then somebody else submits a PR.

@ghost
Copy link

ghost commented Jul 22, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ghost ghost added the CLA Signed label Jul 22, 2016
@sotojuan
Copy link

Yeah that looks fine, no problem! Looks good @dhruska!

@ghost ghost added the CLA Signed label Jul 22, 2016
@dhruska
Copy link
Contributor Author

dhruska commented Jul 22, 2016

@sotojuan and @gaearon, I should have made a note that I was taking a look at this one - that's my fault, my apologies.

@gaearon, sounds great - I suppose we should make a mention of this in the README potentially?

@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

@dhruska Yep, should do this as part of #92.

Can you please look into why travis fails?

@dhruska
Copy link
Contributor Author

dhruska commented Jul 22, 2016

I'm having trouble viewing the build details - getting The repository at facebookincubator/create-react-app was not found.. Potentially a permissions issue?

As for the README do you think we should mention that the prod package url is read from package.json, or put that in the creation flow?

@@ -26,6 +26,7 @@ var nodeModulesPath = path.join(__dirname, '..', 'node_modules');
var indexHtmlPath = path.resolve(__dirname, relativePath, 'index.html');
var faviconPath = path.resolve(__dirname, relativePath, 'favicon.ico');
var buildPath = path.join(__dirname, isInNodeModules ? '../../..' : '..', 'build');
var publicPath = require('./package.json').homepage || '/';
Copy link
Contributor

Choose a reason for hiding this comment

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

package.json might not be there. I think you need to use something like path.resolve(__dirname, relativePath, 'package.json');

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that, but then I run into an error that there is no package.json in the template folder. I can include a sample package.json in that folder and then the tests will successfully run - are you ok with that @gaearon ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Having a dummy package json there sounds reasonable to me.

Choose a reason for hiding this comment

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

THANK U every much, try to config package.json with "homepage":"./" make it work on default app.

@keyz
Copy link
Contributor

keyz commented Jul 22, 2016

Travis log:

screen shot 2016-07-22 at 12 36 48 pm

@ghost ghost added the CLA Signed label Jul 22, 2016
@ghost ghost mentioned this pull request Jul 22, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

I’ll check in with our team about Travis permissions, stay tuned 👍

@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

For now you can run sh ./tasks/e2e.sh

@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

If you push again, Travis should pick this up now. We fixed permissions.

@ghost ghost added the CLA Signed label Jul 22, 2016
@dhruska
Copy link
Contributor Author

dhruska commented Jul 23, 2016

Sounds great - I can access Travis now. Added a sample package.json per our conversation above and fixed the file path.

// TODO: this wouldn't work for e.g. GH Pages.
// Good news: we can infer it from package.json :-)
publicPath: '/'
publicPath: publicPath
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it’s best to infer just the path portion? e.g. stuff.com/wow should give us just /wow.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, agreed - good call. Putting together a regex that could do it, but do you think there is a more elegant way?

Copy link
Contributor

Choose a reason for hiding this comment

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

url.parse() gives you an object with pathname.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right. Thanks for bearing with me. Added that, so we should now always have the path portion of the url defined in homepage, or / if it isn't defined.

@gaearon
Copy link
Contributor

gaearon commented Jul 24, 2016

This looks very good. The only problem I see so far is that our instructions no longer work in this case:

You can now serve it with any static server, for example:
  cd build
  npm install -g http-server
  hs
  open http://localhost:8080

Not sure how to fix this yet.

@gaearon
Copy link
Contributor

gaearon commented Jul 24, 2016

Let’s change the message to

Successfully generated a bundle in the build folder!
You can now deploy and serve it from <homepage>.
The bundle is optimized and ready to be deployed to production.

(only when homepage is set)

@gaearon
Copy link
Contributor

gaearon commented Jul 24, 2016

You will also need to add a section in howto (template/README.md).
Ideally there should be a walkthrough on publishing to GH pages.

console.log(' hs');
console.log(' ' + openCommand + ' http://localhost:8080');
console.log();
console.log('You can now deploy and serve it from <homepage>.');
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep the previous message for the simple case.
This code should check whether the homepage was set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed an update for that - will work on instructions for publishing to GH Pages.

@selfup
Copy link

selfup commented Jul 24, 2016

A great start would be to (after setting the project page homepage key:value):

  • git checkout -b gh-pages (if never created)
  • npm run build
  • git add . && git commit -m "some commit message"
  • git subtree push --prefix build origin gh-pages

@gaearon
Copy link
Contributor

gaearon commented Jul 24, 2016

git subtree push --prefix build origin gh-pages

TIL.

@selfup
Copy link

selfup commented Jul 24, 2016

I learned that today too! Haha

@dhruska
Copy link
Contributor Author

dhruska commented Jul 24, 2016

Wow, that is awesome!

@gaearon gaearon merged commit ef4f8e9 into facebook:master Jul 24, 2016
@ghost ghost added the CLA Signed label Jul 24, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 24, 2016

FYI, I continued this in #162.

@gaearon gaearon added this to the 0.2.0 milestone Jul 25, 2016
gaearon added a commit that referenced this pull request Jul 25, 2016
This way the one generated by `init.js` doesn’t get overwritten later.
This fixes #149 which got broken as part of #94
@gaearon gaearon mentioned this pull request Jul 27, 2016
kalekseev pushed a commit to kalekseev/create-react-app that referenced this pull request Sep 11, 2017
Upgrade to typescript 2.4 and ts-loader 2.2.1
TedChenNZ pushed a commit to TedChenNZ/create-react-app that referenced this pull request Sep 22, 2017
Support custom config on running tests as well (e.g. for decorators)
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
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.

6 participants