Skip to content

serve app under homepage - #4158

Closed
kellyrmilligan wants to merge 8 commits into
react:masterfrom
kellyrmilligan:feature/serve-app-under-homepage
Closed

serve app under homepage#4158
kellyrmilligan wants to merge 8 commits into
react:masterfrom
kellyrmilligan:feature/serve-app-under-homepage

Conversation

@kellyrmilligan

Copy link
Copy Markdown
Contributor

No description provided.

@kellyrmilligankellyrmilligan changed the title new pr against next branchserve app under homepageMar 15, 2018
* next:
Fix typos in example monorepo documentation. (react#4164)
Minor fixes to CI (react#4193)
Change no-unused-vars 'args' from none to all to show warning on destructured objects
Bump babel-related deps (react#4159)
Remove ref to non free resource (react#4160)
@kellyrmilligan

Copy link
Copy Markdown
ContributorAuthor

@Timer for 2.0, this feature, what else do I need to put into this? webpack serve as well? how do we want to proceed?

@TimerTimer added this to the 2.0.0 milestone Mar 26, 2018
@Timer

Copy link
Copy Markdown
Contributor

We'll have a better idea after the webpack-serve cut over. You're more than welcome to take a stab at it if you'd like.

@kellyrmilligan

Copy link
Copy Markdown
ContributorAuthor

So cra is definitely switching to serve for 2.0 then? I guess let me know if you want to get a version working where it’s the same as current next or if this can be merged first then brought over with serve. This feature is blocking all of my apps from being put back into cra ATM. Had to eject to just run them all at the same time behind an Apache proxy and I’d rather not be ejected. It seems like others want this functionality as well. Please let me know the best way I can help with this, as it’s coming up on a full year for this feature.

@jeremygiberson

Copy link
Copy Markdown

How do I support deploying to more than 2 environments (dev/prod)? I've got local(dev), integration, stage, prod. This gives me domains like dev.mysite.com, test.mysite.com and mysite.com.

If that is not supported, is there any chance to move the value out from package.json homepage and into a .env value instead?

@itsjoekent

Copy link
Copy Markdown

Any updates on merging this?

* upstream/next: (35 commits)
Update envinfo and issue template (react#4375)
Update sass-loader to 7.0.1 (react#4376)
Support package distribution tags (react#4350)
fix broken css module support in prod (react#4361)
Bumped jest version to 22.4.1 (react#4362)
bump babel 7 to beta 46
bump lint-staged to node 10 compatible version
documentation: Added License to the README.md (react#4294)
Bump `fsevents`. (react#4331)
Fix typo in e2e-simple.sh comment (react#4323)
Add Sass loader (react#4195)
Fix some typos in README.md (react#4286)
Added learnstorybook.com to Storybook links (react#4298)
Document multiple build environments via `env-cmd` react#4071 (react#4117)
Fixed link to CSS imports blog post
Update CSS Modules localIndetName (react#4192)
Enable loose mode for `class-properties` (react#4248)
bump babel 7 beta (react#4253)
Small typo fixreact#4217
Changelog for 1.1.4
...
* next: (35 commits)
Update envinfo and issue template (react#4375)
Update sass-loader to 7.0.1 (react#4376)
Support package distribution tags (react#4350)
fix broken css module support in prod (react#4361)
Bumped jest version to 22.4.1 (react#4362)
bump babel 7 to beta 46
bump lint-staged to node 10 compatible version
documentation: Added License to the README.md (react#4294)
Bump `fsevents`. (react#4331)
Fix typo in e2e-simple.sh comment (react#4323)
Add Sass loader (react#4195)
Fix some typos in README.md (react#4286)
Added learnstorybook.com to Storybook links (react#4298)
Document multiple build environments via `env-cmd` react#4071 (react#4117)
Fixed link to CSS imports blog post
Update CSS Modules localIndetName (react#4192)
Enable loose mode for `class-properties` (react#4248)
bump babel 7 beta (react#4253)
Small typo fixreact#4217
Changelog for 1.1.4
...
@kellyrmilligan

kellyrmilligan commented May 2, 2018

Copy link
Copy Markdown
ContributorAuthor

just merged the latest next branch. @gaearon@Timer where should I leave this pr? I am moving to a different company shortly, and may not have need of this anymore. I want to see it through, but i'm not really sure where to go next. my main questions are:

a slew of interconnected issues as always 😄

@kellyrmilligan

kellyrmilligan commented May 3, 2018

Copy link
Copy Markdown
ContributorAuthor

@Timer can we put this pr in the 2.0 project list? The old PR was closed and is there, maybe this new one is under the maybe category?

@mcMickJuice

Copy link
Copy Markdown

FYI one problem I ran into with this implementation is proxying api requests through webpack:

const proxy = {
'/scheduling': {
target: 'http://localhost:3001',
pathRewrite: { '^/scheduling': '' },
},
'/api': {
target: 'http://another-api-endpoint',
},
'/app': {
target: 'http://someapp.endpoint',
},
}
const proxyConfig = prepareProxy(proxy, paths.appPublic)

When accessing the app at localhost:3000/route the api requests that should be proxied fail as they are being redirected to servedPathPathName (as dictated in serveAppMiddleware). The workaround here was to pass in the proxyKeys and respect those as well as the servedPathPathName:

module.exports = function createServeAppMiddleware(
servedPathPathname,
proxyKeys = []
) {
function matchesProxyOrServedPathName(requestUrl) {
const matchesServedPathName = requestUrl.indexOf(servedPathPathname) !== -1
const matchesProxy =
matchesServedPathName ||
proxyKeys.some(key => {
return requestUrl.startsWith(key)
})
return matchesServedPathName || matchesProxy
}
return function serveAppMiddleware(req, res, next) {
if (servedPathPathname.length > 1 && servedPathPathname !== './') {
if (matchesProxyOrServedPathName(req.url) === false) {
res.redirect(servedPathPathname)
} else {
next()
}
} else {
next()
}
}
}

Note that I'm running into this issue after I ejected from CRA and I basically lifted all of your changes into my app locally. This might not be an issue with current version of next and these changes but thought I'd mention them in case this use case hasn't been tested.

@kelly-tock

Copy link
Copy Markdown

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, and someone can start anew when the webpack 4 release is cut.

@kelly-tock

Copy link
Copy Markdown

I haven't used CRA in a while, is this supported now officially in the latest release?

@gaearongaearon closed this Nov 1, 2018
@davidroeca

davidroeca commented Nov 1, 2018

Copy link
Copy Markdown

@gaearon was this PR was closed due to this feature being added elsewhere? I’ve been following the related issues over the past few months and haven’t really contributed much to the discussion because I assumed people were working on this, as this PR was mentioned in numerous issues, and it has been periodically added and removed from milestones.

This is honestly the final feature that keeps me from ditching my own custom set ups. It will also free me and likely many others from the awful webpack-dev-server vs webpack-serve war if our custom setups need to rely on a non-root publicPath. Thanks for an awesome project!

@Timer
Timer changed the base branch from next to masterNovember 1, 2018 21:06
@TimerTimer reopened this Nov 1, 2018
@schotime

Copy link
Copy Markdown

Is this PR going to make it in?

@elyran

Copy link
Copy Markdown

+1

@ericclemmons

Copy link
Copy Markdown
Contributor

I happened to toy around with this as well in:

#6135 (comment)

Unfortunately, I got blocked on weird behavior between InterpolateHtmlPlugin not replacing %PUBLIC_URL% when the devServer's publicPath was anything but /.

It seemed so simple at first!

@ericclemmons

Copy link
Copy Markdown
Contributor

#6280 may replace this PR. 👀

@iansuiansu modified the milestones: 2.x, 3.xMar 10, 2019
@locklockBot locked and limited conversation to collaborators Feb 8, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@kellyrmilligan@Timer@jeremygiberson@itsjoekent@mcMickJuice@kelly-tock@davidroeca@schotime@elyran@ericclemmons@iansu@gaearon@facebook-github-bot