-
-
Notifications
You must be signed in to change notification settings - Fork 27k
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
%PUBLIC_URL% in development #6280
%PUBLIC_URL% in development #6280
Conversation
4ed8789
to
b40888e
Compare
Aha! Looks like I can duplicate the tests in |
@ericclemmons with this PR on |
Correct.
… |
Need any help getting this merged? I currently need this for my development environment. In production we have multiple apps being served from different folders, and I would like my development environment to mimic that. e.g. localhost/admin |
We've been waiting for this for a long time. Also happy to help if there are specific to-dos left to work on. Do we know whats causing the build failure? |
I need help from contributors to know if this is ready to merge, or what's
missing.
The failed build seems to be sporadic in CI due to timeouts with NPM.
I don't know what else to do besides wait for approval...
|
@ericclemmons can you try merging in And yes, we would also like to see this feature in development as it enables us to have near 1:1 mapping with our production environment when developing |
@ericclemmons let me know if you need any help on this pull request |
@raix, @ericclemmons is there anything I can do to help to make this feature a reality? Is the entire bottleneck the failing tests? |
@@ -59,19 +59,17 @@ module.exports = function(webpackEnv) { | |||
// Webpack uses `publicPath` to determine where the app is being served from. | |||
// It requires a trailing slash, or the file assets will get an incorrect path. | |||
// In development, we always serve from the root. This makes config easier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the last part of the comment isn't true after this PR? :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// In development, we always serve from the root. This makes config easier. |
@@ -68,7 +68,7 @@ module.exports = function(proxy, allowedHost) { | |||
hot: true, | |||
// It is important to tell WebpackDevServer to use the same "root" path | |||
// as we specified in the config. In development, we always serve from /. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last part of the comment here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// as we specified in the config. In development, we always serve from /. | |
// as we specified in the config. |
I'm unavailable for the next week, so if a contributor can append these
commits, I'd appreciate it.
|
@ericclemmons do you need help addressing review comments? or are you waiting for more reviewers to join? |
I tested these changes (on top of v3 release). It works well but it's tricky to get the assets in public works well. If your homepage (or PUBLIC_URL in my case is It's good if you want to start multiple dev instance with different assets. But not sure it's the main use case and it requires some cp/mv before May be it can be simplify if we differentiate Also some comment could be updated in
|
@ArnaudBarre I see, so we actually have two scenarios to cater for:
We might actually ending up using both since we some times do development in containers and other times use the development server to proxy to the cluster. |
@@ -68,7 +68,7 @@ module.exports = function(proxy, allowedHost) { | |||
hot: true, | |||
// It is important to tell WebpackDevServer to use the same "root" path | |||
// as we specified in the config. In development, we always serve from /. | |||
publicPath: '/', | |||
publicPath: paths.servedPath.slice(0, -1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been messing around with this locally and I have found that
further down in this file needs to be updated as well, something similar to:
historyApiFallback: {
// Paths with dots should still use the history fallback.
// See https://github.com/facebook/create-react-app/issues/387.
disableDotRule: true,
index: paths.servedPath,
},
otherwise when you go to pages other than the home page with react router setup and refresh it will fail to load files.
I suppose it may be complex to handle the two use cases (in near term) but perhaps this PR is a good first step to get out? We've forked this PR at my company because we really needed to set PUBLIC_URL in development. Currently stress testing but seems to be working just fine. It's kinda like the same as it was with a little more control in dev |
Hey guys, we need exact this to be able to use CRA in our projects. |
I would like to be able to use this in my project. What's the status? Can we get this merged? |
Same here, would like to use this in its current form as well.
…________________________________
From: Margo Baxter <[email protected]>
Sent: Monday, May 20, 2019 7:27:37 PM
To: facebook/create-react-app
Cc: Shubhan Chemburkar; Manual
Subject: Re: [facebook/create-react-app] %PUBLIC_URL% in development (#6280)
I would like to be able to use this in my project. What's the status? Can we get this merged?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#6280?email_source=notifications&email_token=AAYDKM4CVJXO5TO2ZBQXXHLPWKU5DA5CNFSM4GSP4ON2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVY47YQ#issuecomment-493998050>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAYDKM35MGCFVACORYYYTK3PWKU5DANCNFSM4GSP4ONQ>.
|
I also want to be able to use this in my project. What is blocking this pull request from being merged? Anything we can help? |
Another thing that might be useful to make the I guess the most convenient way would be through an environment variable, ( The reason I am suggesting this is because in more complex development setups that mimic production it's not always possible to access to Edit: Although, this suggestion might be better suited for another PR. |
Is that fork going to be merged one day? I can not wait for getting public-url to work in development to mimic the same behaviour as in production. |
Let's close the loop and merge! |
As there have already been two approvals for reviews, is this now going to be merged any time soon? I encountered this issue now in a serverless environment using now.sh. Local development is hindered by CRA not supporting PUBLIC_PATH under development environment. |
Listen, anyone can submit a review, so I would take reviews from members outside the team and especially non-contributors with a grain of salt. With that said, there are still unresolved comments, one of which mentions that this PR breaks |
If anyone is interested in funding my time to get this PR resolved, let me
know.
I don't have any free hours remaining since I created this. :(
…On Wed, Jun 19, 2019, 12:43 PM Tim Phillips ***@***.***> wrote:
Listen, anyone can submit a review, so I would take reviews from members
outside the team and especially non-contributors with a grain of salt.
With that said, there are still unresolved comments, one of which mentions
that this PR breaks react-router.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6280?email_source=notifications&email_token=AAADWTWQJJLAE62BW6WW4BTP3KD5RA5CNFSM4GSP4ON2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYDCGII#issuecomment-503718689>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAADWTX3LLRQP7Y3OB6M4TLP3KD5RANCNFSM4GSP4ONQ>
.
|
@ericclemmons If you or the mainainers of CRA don't have the time to fix the comments - we could create a new pull-request and get this ready - but I would like to have a "go" from one of the maintainers that they actually want this change in.. cc @iansu It's fair enough if CRA project is only meant as a helping hand to newcommers to get started with react and not for advanced usage/developers/containers etc. @tim-phillips I agree that the code owners should review - but the react router issue is a react router issue - in the sense that it's solvable by the developer? eg. you could set the base url to match what ever your |
I always use this code to initialize router history const history = createBrowserHistory({
basename: process.env.PUBLIC_URL
}) |
I have the time to take care of this. I made a new PR with all this changes, and review changes. |
Closing in favor of #7259 |
In response to #6135 (and possibly replacing #4158), I'm attempting to enable support for
homepage
in development as well.This way, development matches production URLs, which is especially necessary when using CRA as part of a larger app, or behind a proxy.
I've been able to record a video of this working:
I could use some help on how I could further test this, & to what degree.