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

dhruska · 2016-07-22T19:17:34Z

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 · 2016-07-22T19:18:07Z

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 · 2016-07-22T19:20:15Z

@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 · 2016-07-22T19:22:23Z

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

sotojuan · 2016-07-22T19:27:37Z

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

dhruska · 2016-07-22T19:28:39Z

@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 · 2016-07-22T19:29:28Z

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

Can you please look into why travis fails?

dhruska · 2016-07-22T19:32:13Z

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?

keyz · 2016-07-22T19:36:16Z

config/webpack.config.prod.js

@@ -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 || '/';


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

This sounds right

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 ?

Having a dummy package json there sounds reasonable to me.

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

keyz · 2016-07-22T19:37:12Z

Travis log:

gaearon · 2016-07-22T19:46:32Z

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

gaearon · 2016-07-22T19:46:41Z

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

gaearon · 2016-07-22T20:16:22Z

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

dhruska · 2016-07-23T00:31:11Z

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

gaearon · 2016-07-23T18:54:55Z

config/webpack.config.prod.js

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


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

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

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

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.

…page

gaearon · 2016-07-24T21:10:42Z

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 · 2016-07-24T21:18:14Z

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 · 2016-07-24T21:19:08Z

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

gaearon · 2016-07-24T21:23:13Z

scripts/build.js

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


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

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

…in package.json

selfup · 2016-07-24T22:04:37Z

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 · 2016-07-24T22:06:04Z

git subtree push --prefix build origin gh-pages

TIL.

selfup · 2016-07-24T22:12:43Z

I learned that today too! Haha

dhruska · 2016-07-24T22:14:17Z

Wow, that is awesome!

gaearon · 2016-07-24T23:31:53Z

FYI, I continued this in #162.

This way the one generated by `init.js` doesn’t get overwritten later. This fixes #149 which got broken as part of #94

Upgrade to typescript 2.4 and ts-loader 2.2.1

Support custom config on running tests as well (e.g. for decorators)

Don't assume the project is hosted at the root

0081f76

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

ghost added the CLA Signed label Jul 22, 2016

keyz reviewed Jul 22, 2016
View reviewed changes

ghost added the CLA Signed label Jul 22, 2016

ghost mentioned this pull request Jul 22, 2016

Deployment with Surge or similar #84

Closed

ghost added the CLA Signed label Jul 22, 2016

Fix package.json path and add sample package.json for tests

17ced6b

gaearon reviewed Jul 23, 2016
View reviewed changes

Update publicPath to use relative path portion of URL defined in home…

24c9c9b

…page

Update successful bundle generation message

e2dc775

gaearon reviewed Jul 24, 2016
View reviewed changes

Show bundle generation success message based on presence of homepage …

d16a092

…in package.json

gaearon merged commit ef4f8e9 into facebook:master Jul 24, 2016

ghost added the CLA Signed label Jul 24, 2016

gaearon mentioned this pull request Jul 24, 2016

Provide instructions on publishing GH Pages #162

Merged

vgrigoriu mentioned this pull request Jul 25, 2016

Relative paths in build output #165

Closed

gaearon added this to the 0.2.0 milestone Jul 25, 2016

gaearon added a commit that referenced this pull request Jul 25, 2016

Delete package.json from template

750472b

This way the one generated by `init.js` doesn’t get overwritten later. This fixes #149 which got broken as part of #94

gaearon mentioned this pull request Jul 27, 2016

Changelog for 0.2.0 #190

Merged

kalekseev pushed a commit to kalekseev/create-react-app that referenced this pull request Sep 11, 2017

Merge pull request facebook#94 from frederickfogerty/master

7395212

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

Merge pull request facebook#94 from dforsten/master

19fe8e1

Support custom config on running tests as well (e.g. for decorators)

csepulv mentioned this pull request Feb 22, 2018

images broken after building with electron-builder csepulv/electron-with-create-react-app#19

Closed

lock bot locked and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dhruska commented Jul 22, 2016

ghost commented Jul 22, 2016

gaearon commented Jul 22, 2016

ghost commented Jul 22, 2016

sotojuan commented Jul 22, 2016

dhruska commented Jul 22, 2016

gaearon commented Jul 22, 2016

dhruska commented Jul 22, 2016

keyz Jul 22, 2016

gaearon Jul 22, 2016

dhruska Jul 22, 2016

gaearon Jul 22, 2016

aaniao002 Nov 18, 2018

keyz commented Jul 22, 2016

gaearon commented Jul 22, 2016

gaearon commented Jul 22, 2016

gaearon commented Jul 22, 2016

dhruska commented Jul 23, 2016

gaearon Jul 23, 2016

dhruska Jul 24, 2016

gaearon Jul 24, 2016

dhruska Jul 24, 2016

gaearon commented Jul 24, 2016

gaearon commented Jul 24, 2016 •

edited

Loading

gaearon commented Jul 24, 2016

gaearon Jul 24, 2016

dhruska Jul 24, 2016

selfup commented Jul 24, 2016 •

edited

Loading

gaearon commented Jul 24, 2016

selfup commented Jul 24, 2016

dhruska commented Jul 24, 2016

gaearon commented Jul 24, 2016

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

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

Conversation

dhruska commented Jul 22, 2016

ghost commented Jul 22, 2016

gaearon commented Jul 22, 2016

ghost commented Jul 22, 2016

sotojuan commented Jul 22, 2016

dhruska commented Jul 22, 2016

gaearon commented Jul 22, 2016

dhruska commented Jul 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keyz commented Jul 22, 2016

gaearon commented Jul 22, 2016

gaearon commented Jul 22, 2016

gaearon commented Jul 22, 2016

dhruska commented Jul 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Jul 24, 2016

gaearon commented Jul 24, 2016 • edited Loading

gaearon commented Jul 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfup commented Jul 24, 2016 • edited Loading

gaearon commented Jul 24, 2016

selfup commented Jul 24, 2016

dhruska commented Jul 24, 2016

gaearon commented Jul 24, 2016

gaearon commented Jul 24, 2016 •

edited

Loading

selfup commented Jul 24, 2016 •

edited

Loading