Skip to content

Conversation

unrevised6419
Copy link

@unrevised6419 unrevised6419 commented Jun 21, 2019

Allow PUBLIC_URL in development mode

Public API

Private API

  • Combined paths.publicUrl and paths.servedPath into paths.publicUrlOrPath
  • Extracted all logic into react-dev-utils/getPublicUrlOrPath (no side effects)
  • Moved evalSourceMapMiddleware and errorOverlayMiddleware first in the middleware chain, redirect does not affect them.
  • Moved proxy middleware after redirect (this is the case what most want, proxy should respect PUBLIC_URL
  • Adapted noopServiceWorkerMiddleware to serve from servedPath
  • Updated webpack-dev-server@3.9.0 to webpack-dev-server@3.10.0

Blocked by webpack/webpack-dev-server/pull/2150
Blocked by webpack/webpack-dev-server/pull/2374
Blocked by https://github.com/webpack/webpack-dev-server new patch release

Closes #6280
Closes #6135
Closes #4158

@unrevised6419 unrevised6419 changed the title <!-- Thank you for sending the PR! feat(react-scripts): allow PUBLIC_URL in develoment mode Jun 21, 2019
Copy link

@ArnaudBarre ArnaudBarre left a comment

Choose a reason for hiding this comment

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

packages/react-scripts/config/webpack.config.js L. 561-563 to update

@unrevised6419
Copy link
Author

@ArnaudBarre done!

Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

@iansu or @ianschmitz - can I get your thoughts on this one?

@mrmckeb mrmckeb self-assigned this Jun 26, 2019
Copy link

@jtblanche jtblanche left a comment

Choose a reason for hiding this comment

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

this fixes the issue I mentioned at #6280 (comment) 👍

@64BitAsura
Copy link

64BitAsura commented Jul 13, 2019

@iansu @Timer @gaearon @amyrlam @ianschmitz @petetnt @bugzpodder please review this PR, if all good merge it ASAP and make release please 🙏 , my projects requires and also I don't want to eject since we don't have enough resource to setup packer env 😢

@mrmckeb
Copy link
Contributor

mrmckeb commented Jul 15, 2019

@sambathl I'm chasing a second review on this, and then we can merge it in.

@heyimalex
Copy link
Contributor

I don't think it works when "homepage": ".".

@heyimalex
Copy link
Contributor

I think all that needs to change is webpackDevServer.config.js needs to check for the relative path when setting publicPath.

publicPath: paths.servedPath === "./" ? "/" : paths.servedPath.slice(0, -1)
@unrevised6419
Copy link
Author

unrevised6419 commented Jul 15, 2019

There is also a problem when setting homepage /something and opening /,
manifest.json and favicon.ico return 404 trying to load http://localhost:3000/%PUBLIC_URL%/manifest.json and http://localhost:3000/%PUBLIC_URL%/favicon.ico

Edit: In this case dev server just served clear index.html content without injecting dynamic vars

@heyimalex
Copy link
Contributor

@iamandrewluca Hm, I'm actually not getting that anymore with my changes, so maybe the PUBLIC_URL thing was fixed by ad7e29b? But the dev server is definitely serving index.html when requests for favicon.ico are made if homepage is set.

@raix
Copy link
Contributor

raix commented Jan 13, 2020

@iamandrewluca not sure if merging in master will fix the build issue / or at least rerun the build?

@unrevised6419
Copy link
Author

unrevised6419 commented Jan 13, 2020

@raix made a rebase to trigger the build. It seems there is a babel plugin dependency error comming from master.

@lucasmogari
Copy link

I'm trying this branch. I'm curious about why the public url have to end with a slash?

If I set the public_url with for example: "PUBLIC_URL=/test" and browse http://localhost:3000/test, it redirects the url to: http://localhost:3000/test/test

Btw, thanks for this PR, it's very useful.

@unrevised6419
Copy link
Author

unrevised6419 commented Jan 14, 2020

If I set the public_url with for example: "PUBLIC_URL=/test" and browse http://localhost:3000/test, it redirects the url to: http://localhost:3000/test/test

It should not, I'll check.

@lucasmogari
Copy link

Another thing I noticed is that when reloading http://localhost:3000/test/some-path, it returns a 404 error. Reloading http://localhost:3000 works.

I don't know if it's related, but I'm running the apps behind a reverse proxy (nginx).

listen 3000; # App with PUBLIC_URL=/test location /test { proxy_pass http://localhost:3020; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; } location / { proxy_pass http://localhost:3010; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; } 
@unrevised6419
Copy link
Author

@lucasmogari on http://localhost:3000 is your nginx proxy?

@raix
Copy link
Contributor

raix commented Jan 15, 2020

@lucasmogari shouldn't it be:

location /test { proxy_pass http://localhost:3020/test; 
@lucasmogari
Copy link

It actually works both ways. According to nginx docs:

https://docs.nginx.com/nginx/admin-guide/web-server/reverse-proxy/

If the address is specified without a URI, or it is not possible to determine the part of URI to be replaced, the full request URI is passed

The refresh problem happens in both ways too. I think there's something related to what is described in the docs, but there's no guide for nginx.

https://create-react-app.dev/docs/deployment/#serving-apps-with-client-side-routing

@lucasmogari
Copy link

lucasmogari commented Jan 15, 2020

@lucasmogari on http://localhost:3000 is your nginx proxy?

Yes. What I'm trying to do:

http://localhost:3000 proxies to http://localhost:3010/ (main app)
http://localhost:3000/test proxies to http://localhost:3020/test (test app)

In dev, refreshing http://localhost:3000/some-path works, but http://localhost:3000/test/some-path returns 404.

@lucasmogari
Copy link

I found that adding the option (index: paths.publicUrlOrPath) to webpackDevServer.config.js fixed the problem with refreshing:

historyApiFallback: { // Paths with dots should still use the history fallback. // See https://github.com/facebook/create-react-app/issues/387. disableDotRule: true, index: paths.publicUrlOrPath, }, 
@raix
Copy link
Contributor

raix commented Jan 21, 2020

Just a kind reminder - in 5 days / the 26th of January the original pr for this pull-request turns 1 year. Not often pull-requests live this long so I'm not sure if I should bring cake or flowers? :)

(Disclaimer: I'm not trying to push or rush things - even if we desperately want this to be merged)

@unrevised6419
Copy link
Author

@raix 😄 will live till will be merged 🌹 🌷 💮

@andriijas
Copy link
Contributor

@iamandrewluca Can you rebase locally and push?

@unrevised6419
Copy link
Author

unrevised6419 commented Jan 31, 2020

@lucasmogari I addressed the issues you encoutnered, can you try again with the proxy?

/test now will not redirect to /test/
/test/some-path returns 200

@andriijas done!

@unrevised6419
Copy link
Author

unrevised6419 commented Jan 31, 2020

PR Blocked by a webpack-dev-server patch release
webpack/webpack-dev-server/pull/2374

@andriijas
Copy link
Contributor

@iamandrewluca In case we forget, ping when webpack-dev-server releases a new version! Thanks for your patience!

@unrevised6419
Copy link
Author

Ok, I'll ping. Contacted a webpack maintaner for an ETA.

@lucasmogari
Copy link

@lucasmogari I addressed the issues you encoutnered, can you try again with the proxy?

/test now will not redirect to /test/
/test/some-path returns 200

With PUBLIC_URL=/test
/test is redirecting (returns 302) to /test/test
/test/some-path (returns 200)

With PUBLIC_URL=/test/
/test/ (returns 200)

Maybe it's some configuration here. I'm sorry, but I don't have too much time to debug right now.

Thanks again!

@unrevised6419
Copy link
Author

unrevised6419 commented Jan 31, 2020

@lucasmogari Try in incognito mode. Or clear all data for localhost:3000
For me when I tested also redirected to /test/.
If previous was any 301 redirect from localhost:3000/test to localhost:3000/test/, browser remembers that, and will always redirect.

If I open localhost:3000/test in incognito mode, it stays on this path.

Co-authored-by: Eric Clemmons <eric@smarterspam.com> Co-authored-by: Alex Guerra <alex@heyimalex.com> Co-authored-by: Kelly <kelly.milligan@gmail.com>
@unrevised6419
Copy link
Author

@andriijas all done! Need review.

@andriijas andriijas merged commit 1cbc6f7 into facebook:master Feb 2, 2020
@andriijas
Copy link
Contributor

Thanks for your patience everyone, specially @iamandrewluca

If anyone wants to do additional testing on their projects - please do!

@kelly-tock
Copy link

Well done!

@unrevised6419 unrevised6419 deleted the public-url-in-development branch February 2, 2020 21:29
@unrevised6419
Copy link
Author

unrevised6419 commented Feb 2, 2020

I encountered some problems when using proxy with this feature. Investigating. Think I should reorder in a right way proxy middlewares.

Also one case
Having /test homepage, any fetch ex: /api/resource will redirect to /test/api/resource which I think is wrong.

Do we revert the PR, and create a new one with fixed problems, or we fix the problem till the next release?

@unrevised6419
Copy link
Author

unrevised6419 commented Feb 3, 2020

Found the issue. Will make a PR soon. Internal proxy that decides to proxy or not, does not know about public path

@lock lock bot locked and limited conversation to collaborators Feb 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.