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

serve bundles and app under homepage path #1887

Closed
wants to merge 18 commits into from

Conversation

kellyrmilligan
Copy link
Contributor

@kellyrmilligan kellyrmilligan commented Mar 23, 2017

Verified by adding homepage to package.json, and confirming that it would display the subpath in the terminal, and open the browser with the sub path in the url.

tested by editing both js and css to verify hot loading works the same as before.

Then verified that if I take homepage back out of package.json, it works as intended.

screen shot 2017-03-23 at 9 21 51 am
screen shot 2017-03-23 at 9 21 40 am

@facebook-github-bot
Copy link

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!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@kellyrmilligan
Copy link
Contributor Author

#1582

@facebook-github-bot
Copy link

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

@smmoosavi
Copy link

with this pr can we pass full url as PUBLIC_URL?

example:
code:

import logo from './logo.svg';
console.log(logo);
console.log(process.env.PUBLIC_URL);

command:

PUBLIC_URL=http://localhost:3000 npm run start

what I expect:

"http://localhost:3000/static/media/logo.5d5d9eef.svg"
"http://localhost:3000"

#1525 (comment)

@Timer
Copy link
Contributor

Timer commented Mar 31, 2017

Can you please re-add the relevant tests from #937?

@kellyrmilligan
Copy link
Contributor Author

@smmoosavi that command currently doesn't work off of the master branch either, so i'm not expecting it to work with this. with that command, off of master even, it tries to open the browser with http://localhost:3000http://localhost:3000/

but the result when you open the correct url is

http://localhost:3000/static/media/logo.5d5d9eef.svg
App.js:8 http://localhost:3000

under this branch, minus the issue of the command, it prints out

http://localhost:3000/static/media/logo.5d5d9eef.svg
App.js:8 http://localhost:3000

and when you add a homepage with a sub folder:

/subpath/static/media/logo.5d5d9eef.svg
bundle.js:26127 /subpath

@kellyrmilligan
Copy link
Contributor Author

I synced with upstream master. @smmoosavi @Timer is that command supposed to work then?

@kellyrmilligan
Copy link
Contributor Author

@Timer what tests are you referring too? it looks like i'm up to date with master, and #937 was merged as part of #1504, so what am I missing? 😕

@smmoosavi
Copy link

it tries to open the browser with http://localhost:3000http://localhost:3000/

It's ok for me. In development I always close localhost:3000 (cra page) and open localhost:8000 manually (Django page).

I use cra only for static files (e.g. http://localhost:3000/static/js/bundle.js)

@Timer
Copy link
Contributor

Timer commented Mar 31, 2017

@kellyrmilligan there are tests in that PR that were never merged which check for PUBLIC_URL to be respected on npm start calls

@smmoosavi
Copy link

smmoosavi commented Apr 1, 2017

it tries to open the browser with http://localhost:3000http://localhost:3000/

another solution: adding condition only if paths.servedPath starts with / open browser. (else print some text to consle)

@kellyrmilligan
Copy link
Contributor Author

@Timer I looked through the changes in #937 and it seems like all the tests are there? I found one difference, pushing that up. it was just adding an env variable to one a few of the test runs, but not sure if that is desired or not.

@Timer Timer mentioned this pull request May 5, 2017
32 tasks
@Timer Timer added this to the 0.10.0 milestone May 11, 2017
@gaearon
Copy link
Contributor

gaearon commented May 14, 2017

What should happen if I visit http://localhost:3000/ or http://localhost:3000/anotherpath? I think it would be sensible to automatically redirect to the served path instead in this case.

@gaearon
Copy link
Contributor

gaearon commented May 14, 2017

Visiting a subpath like http://0.0.0.0:3000/wow/asd/ appears broken (I set http://lol.github.io/wow as the homepage so http://0.0.0.0:3000/wow works):

screen shot 2017-05-14 at 18 57 38

This breaks any apps relying on client-side routing.

@gaearon
Copy link
Contributor

gaearon commented May 14, 2017

Files in the public folder still appear to be served from root. This seems like a problem because now the app is not truly self-contained, and this will not match the production behavior.

I would expect that the files in the public folder are served from the subdirectory instead.

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.

I’d love to get this feature in but we need to fix the bugs:

  • Subpaths below the main subpath shouldn't 404
  • public folder should serve from main subpath rather than from the root
  • Opening the root should redirect to the main subpath

It is important that you test this pull request thoroughly, and carefully check how different existing CRA features are affected by the new behavior.

@gaearon
Copy link
Contributor

gaearon commented May 14, 2017

It also appears that the end-to-end test is failing. I'm not sure why, but it looks like it never prints The app is running at: for some reason (or maybe the script is doing the wrong thing). I encourage you to try to run it locally and see why it stumbles.

Anyway, I appreciate your work on this so far! I'm afraid I'll have to push this back to 0.11 because I need to cut 0.10 very soon, and unfortunately this feature is not ready.

@gaearon gaearon modified the milestones: 0.11.0, 0.10.0 May 14, 2017
@sergiocruz
Copy link

There's yet another use case I'd like to bring up if possible: the /sockjs-node path.

Do you all think it is possible to namespace that as well?

Right now, it's still trying to find it under root, which unfortunately does not work with my reverse-proxy set up.

Thanks :)

@gaearon
Copy link
Contributor

gaearon commented May 15, 2017

Good point, I agree any root paths should be namespaced in this case. (Including the new stack frame overlay path.)

@kellyrmilligan
Copy link
Contributor Author

kellyrmilligan commented May 15, 2017 via email

@kellyrmilligan
Copy link
Contributor Author

ok, I have the following working locally:

  • when you visit the root, it will redirect to the subpath that you defined. I think that's fair, but not sure if you type in some other random sub path if it should redirect.
  • serving static files from public/subpath - this also means the user would need to create the folder that matches the sub path. I will update the user guide.
  • added the config in webpack dev server to repoint the historyapi fallback to use the servedPath so SPA urls will work now, good catch(totally my bad)

going to run the tests and fix the conflict and push back up.

@gaearon
Copy link
Contributor

gaearon commented May 16, 2017

I guess I'm okay if we serve 404 on other paths. As long as it's not a 500.

@gaearon
Copy link
Contributor

gaearon commented May 16, 2017

this also means the user would need to create the folder that matches the sub path

Hmm I don't think so. It should match the way you expect it to work in production. The public is treated as a root but served as a subpath.

@Timer
Copy link
Contributor

Timer commented Jan 9, 2018

It's been so long I'm not sure I fully understand the entire problem here anymore [or my old concerns].
A quick review of the code looks good, but it seems we're serving the files ourselves now (which is fine). Did we disable the dev server from serving them, or just offer another point of entry?

I remember we had concerns on how homepage and PUBLIC_URL function with this change and just in the whole big picture.

At this point in the game, I think it's best to get this to a functioning point, merge it, and then identify pain points and resolve them in a future breaking release.

@kellyrmilligan
Copy link
Contributor Author

I’m using this right now in day to day dev team of 3, and its working well. Again the main drawback is if you’re using behind a proxy and a sub path, then auto refresh won’t work. I’ll look into the response above to see if there’s more I can do

@Timer
Copy link
Contributor

Timer commented Jan 9, 2018

It seems they're killing off webpack-dev-server -- maybe it'd be best to make our own version with the functionality we need?

@shellscape
Copy link

@Timer not a correct assumption there 😄 webpack-dev-server will exist in perpetuity and continue to accept fixes.

@Timer
Copy link
Contributor

Timer commented Jan 9, 2018

Well that's good!

webpack-dev-server has entered a maintenance-only mode. We won't be accepting any new features or major modifications. We'll still welcome pull requests for fixes however, and will continue to address any bugs that arise. Announcement with specifics pending.

Can we get a small expansion on this, or is it a secret? 😛

@shellscape
Copy link

@Timer DM me on twitter or invite me to a slack and I'll give you all the dirty deets

@kellyrmilligan
Copy link
Contributor Author

@shellscape cool to see a clean break and re arch of web pack dev server. From what I can tell with the current wds, we can’t establish a sock js node connection under the root, is that correct?

@shellscape
Copy link

@kellyrmilligan its like fight club. what's the first rule 😄

The sockJS implementation is a mess. It takes the best guess as what it should be set at, but there are bugaboos all over the place with that. I'd like to forget about it. It haunts my dreams 👻

@kellyrmilligan
Copy link
Contributor Author

Ok. So @Timer and @Gaeron I’d like to re sync this Pr with master, and merge if you agree. It only affects folks that want to proxy cra apps under a sub path behind a web server, seems reasonable to me until the new hotness is released and all our problems magically go away #silverbullet

@kellyrmilligan
Copy link
Contributor Author

Meant @gaearon

@Timer
Copy link
Contributor

Timer commented Jan 10, 2018

Again the main drawback is if you’re using behind a proxy and a sub path, then auto refresh won’t work.

I think I'm fine with this trade-off on first-ship since we're going to coordinate our release very tightly with the new hotness. I don't think we're really worse off than our current position anyway (it just doesn't work now, so we might as well half work).
I'd also like to get the ball rolling that works with the new hotness so it's lined up and ready to go.

@kellyrmilligan
Copy link
Contributor Author

@Timer synced with master just now.

@shellscape
Copy link

@kellyrmilligan @Timer fwiw webpack-serve is now public and kicking. significant minor release on the horizon as well.

@Timer
Copy link
Contributor

Timer commented Mar 7, 2018

@shellscape I'm subscribed to the repo and have been keeping a close eye. I'm sorry for not being more responsive; I'm going through a role change at work right now so my work is sort of doubled down until the transition is complete, thus a lack of time.
I feel strongly about getting this into the 2.0 release -- it has been a pain point for a long time.

@shellscape
Copy link

@Timer zero worries my dude! taker care of that IRL and if the opportunity presents, let me know when and if I can help.

@kellyrmilligan
Copy link
Contributor Author

@Timer do you want to use the new serve module for 2.0 or to get my pr updated for this next release then do it?

@kellyrmilligan
Copy link
Contributor Author

this is now in #4158

@peterbraden
Copy link

What was the status of this? The PR mentioned seems to have died, and this got moved to 'Nope' ?

@bugzpodder
Copy link

@peterbraden See #4158.

@peterbraden
Copy link

Hi, I was referring to the last comment on that PR: "Thanks for this. I am at a new job now, and with webpack 4 this PR is now old and need to be redone. I think at this point i need to let this PR die yet again"

Not sure how the issues tracker works on this project, but if the issue is closed and the PR dies I wanted to ask if this would fall through the cracks?

Thanks!

@bugzpodder
Copy link

I believe at some point create-react-app is going to support this feature but there is no ETA. #4158 is part of the milestone 2.0.0 so rest assured it's not going to fall through the cracks :)

@ram-nadella
Copy link

Hi, did this feature make it into 2.0.0? From what I can tell, doesn't look like it did. (I am running react-scripts@2.1.3 and setting the homepage does not appear to have any affect when serving in dev and no mention of this working in the docs either.

@bugzpodder do you have any insight into this?

@lock lock bot locked and limited conversation to collaborators Jan 16, 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.