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

feat(react-scripts): allow PUBLIC_URL in develoment mode #7259

Merged
merged 1 commit into from
Feb 2, 2020
Merged

feat(react-scripts): allow PUBLIC_URL in develoment mode #7259

merged 1 commit into from
Feb 2, 2020

Conversation

iamandrewluca
Copy link
Contributor

@iamandrewluca iamandrewluca 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

@iamandrewluca iamandrewluca 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

@iamandrewluca
Copy link
Contributor 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)

@iamandrewluca
Copy link
Contributor Author

iamandrewluca 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

@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?

@iamandrewluca
Copy link
Contributor Author

iamandrewluca 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.

@iamandrewluca
Copy link
Contributor Author

iamandrewluca 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;
}

@iamandrewluca
Copy link
Contributor 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)

@iamandrewluca
Copy link
Contributor Author

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

@andriijas
Copy link
Contributor

@iamandrewluca Can you rebase locally and push?

@iamandrewluca
Copy link
Contributor Author

iamandrewluca 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!

@iamandrewluca
Copy link
Contributor Author

iamandrewluca 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!

@iamandrewluca
Copy link
Contributor 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!

@iamandrewluca
Copy link
Contributor Author

iamandrewluca 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>
@iamandrewluca
Copy link
Contributor 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!

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

iamandrewluca 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?

@iamandrewluca
Copy link
Contributor Author

iamandrewluca 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support PUBLIC_URL during development as well