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 app under homepage #4158

Conversation

kellyrmilligan
Copy link
Contributor

No description provided.

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

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

@Timer Timer added this to the 2.0.0 milestone Mar 26, 2018
@Timer
Copy link
Contributor

Timer commented Mar 26, 2018

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
Contributor Author

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

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

Any updates on merging this?

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

kellyrmilligan commented May 2, 2018

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
Copy link
Contributor Author

kellyrmilligan commented May 3, 2018

@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

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

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

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

@gaearon gaearon closed this Nov 1, 2018
@davidroeca
Copy link

davidroeca commented Nov 1, 2018

@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 master November 1, 2018 21:06
@Timer Timer reopened this Nov 1, 2018
@schotime
Copy link

Is this PR going to make it in?

@elyran
Copy link

elyran commented Dec 20, 2018

+1

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

#6280 may replace this PR. 👀

@iansu iansu modified the milestones: 2.x, 3.x Mar 10, 2019
@lock lock bot locked and limited conversation to collaborators Feb 8, 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.