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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify our "advanced" proxy #3366

Closed
Timer opened this issue Oct 31, 2017 · 24 comments
Closed

Simplify our "advanced" proxy #3366

Timer opened this issue Oct 31, 2017 · 24 comments

Comments

@Timer
Copy link
Contributor

Timer commented Oct 31, 2017

I think we made a mistake when we added advanced proxy configuration in 1.x.

It's poor DX imo, and has caused numerous issues (I'm too lazy to search for them 馃槤).
I mean look at this monstrosity. Yuck!

Note: this is not an attack against the contributor of this change. We are very grateful for the contribution and it has served us well. 馃槃

I propose we support the simplest of configurations:

  1. Our default, simple behavior which works for many cases:
      "proxy": "http://localhost:4000",
  2. An advanced case, which uses globs and proxies in 100% of cases (no heuristic):
      "proxy": [
        { "match": ["api/**/*", "!api/ws/*"], "target": "http://localhost:4000" },
        { "match": "api/ws/*", "target": "ws://localhost:4000" },
        { "match": "api2/**/*", "target": "http://localhost:4001" }
      ]
    or
      "proxy": [
        [["api/**/*", "!api/ws/*"], "http://localhost:4000"],
        ["api/ws/*", "ws://localhost:4000"],
        ["api2/**/*", "http://localhost:4001"]
      ]

In the "advanced" case, we don't need to set ssl -- check for https:// over http://. We don't need to set ws, check for ws://; etc.
Other features (e.g. url rewrites) should never happen at the client layer because this doesn't translate to production (it's leaky domain logic).

Thoughts?

We can add some code to auto-migrate old usage and remove it in v3.

@samithaf
Copy link

@Timer wondering that how to configure the option.agent prop with create react app. The reason why I need this option is that I am behind the cooperate proxy and trying to hit API endpoint in outside the cooperate network. Thanks!

@snardone
Copy link

snardone commented Jan 9, 2018

@Timer, the new approach looks good but I have one request based on issues we've run into with our team. It would be helpful if the target value could be configurable outside of package.json.

In our environment, developers can test against the back-end running locally, or against a VM, or against a cloud server. So they modify their package.json accordingly, which gets accidentally checked it, which leads to conflicts, etc.

One solution might be to support environment variables in the target, for example:

"proxy": [
    { "match": ["api/**/*", "!api/ws/*"], "target": "http://${PROXY_HOST}" },
    { "match": "api/ws/*", "target": "ws://${PROXY_HOST}" },
    { "match": "api2/**/*", "target": "http://${PROXY2_HOST}" }
  ]

Thanks!

@samithaf
Copy link

samithaf commented Jan 9, 2018

I think that If you enable user to pass the proxy config as follows then it will handle all the use cases. config object can have any configuration that http-proxy-middleware accepts. So if people want to use features like pathRewrite they can simply use it.

{
	"proxy": [{
			"url": "https://example.com/api",
			"config": {}
		},
		{
			"url": "https://secret.com.au/api",
			"config": {}
		}
	]
}

@FezVrasta
Copy link
Contributor

My need is:

Right now we can define some proxied paths using the package.proxy property, but there's not (AFAIK) a way to tell the app to do something like this:

/ # redirect to localhost:3001
/CRA # serve localhost:3000 (CRA)

In my own project I managed to get this to work with react-app-rewired and the following changes:

HtmlWebpackPlugin({
  inject: true,
  template: './public/index.html',
  filename: 'app.html',
});

and changing the historyApiFallback to { index: '/app.html' }.

The first change is needed because otherwise the proxy will be bypassed if your html file is called index.html (because webpack-dev-server fallbacks to it automatically ignoring the proxy rules).

The second change is needed to tell webpack-dev-server to serve app.html instead of index.html.

Once you have these changes, you are able to write proxy rules like this:

{
  // ...
  "homepage": "/CRA",
  "proxy": {
    "/": {
      "target": "http://localhost:3001",
      // ...
    },
    "/CRA": {
      "target": "http://localhost:3000" // or maybe have a way to tell `bypass` to return `false`?
      // ...
    },
  },
  // ...
}

Is this something you'd like to support?

@maxhallinan
Copy link

maxhallinan commented Jan 10, 2018

@gaearon asked me to continue #3204 on this issue. To summarize:

  • Proxy configuration is part of package.json.
  • package.json is usually (maybe always) under source control.
  • The location of a proxy target can vary with the environment.
  • Because the proxy configuration is under source control, the location of the proxy target must be aligned across environments.
  • Thus, it would be nice to move proxy configuration to the environment.

@gaearon
Copy link
Contributor

gaearon commented Jan 10, 2018

Does anyone want to work on a proposal that would address the above issues together?

It could even be something radical like detecting proxy.js in the project root and just using that as a proxy.

@samithaf
Copy link

@maxhallinan @gaearon I am currently working on a large banking app which has 5 + different environments before code land in to Prod. My project is not on React but using webpack and node http proxy to enable front end engineers to hit any of those environments from their local computer. This is how I handle multiple env problem.
E.g.

npm run dev -- --env uat

When developer runs the above code I get the URL mapping for UAT and create all the required proxy mappings. Alternatively developer can pass a valid http url as the end point since we allow people to spin up backend in cloud too.

Thanks.

@JustFly1984
Copy link

Yeah, 3245 was my concern. I had to eject CRA and get rid of proxy completely.
It really could be useful if proxy could be disabled by default. It was a bummer for me, and I lost couple days to figure out that the issue was a proxy, that CRA uses proxy by default ( it is kind of not obvious, like service workers ), and it took me only couple hours to eject and get rid of proxy fro all configs.

Is it possible to add something like

  "proxy": {
     "enabled": false 
  }

in package.json by default, and write documentation and deprecation warnings for everybody who is updating CRA or new users.

Anyway no one can't predict the backend API chosen by end user.

PS: Thanx everybody for a great work. Even just ejecting CRA saving a ton of work for advanced developers like me. Please keep up a great work!

@gaearon
Copy link
Contributor

gaearon commented Jan 14, 2018

It really could be useful if proxy could be disabled by default.

I'm not sure what you mean. Proxy is not enabled unless you add a proxy key to package.json.

Let me reopen #3245 because it seems like there is either a legitimate bug or a misunderstanding there.

@Timer Timer closed this as completed Jan 14, 2018
2.0 automation moved this from Blocker to Ready Jan 14, 2018
@Timer Timer reopened this Jan 14, 2018
@Timer Timer moved this from Ready to Blocker in 2.0 Jan 14, 2018
@samithaf
Copy link

@gaearon @Timer I am happy to propose a proxy solution and if everyone fine with it I can probably implement the solution as well.

@iansu
Copy link
Contributor

iansu commented Jan 14, 2018

I was just thinking about this as well. I think points 1 and 2 (first example) from @Timer's original comment make sense. I also like @gaearon's idea of looking for proxy.js and using that. So the different options from simplest to most complex would be:

  1. proxy string in package.json
  2. proxy object with multiple patterns in package.json
  3. proxy.js file where you can do any routing you want

Option 3 is interesting because of the potential for advanced use cases. It also allows the first two options to be simpler/less configurable. I'm not exactly sure how option 3 would work. I guess you would just be providing a custom router function for http-proxy-middleware (https://github.com/chimurai/http-proxy-middleware#http-proxy-middleware-options)

@samithaf
Copy link

I would vote for proxy.js and this allows people to use any option http-proxy-middleware gives also we can add multiple environment support as well. E.g. UAT, SIT and PROD

@samithaf
Copy link

@gaearon @Timer @iansu requesting your feedback.

Proposal Iteration 01:

As a create react app user I want to configure proxy middleware easily so that I can connect with API endpoints without getting browser security warnings.

New proxy solution will add the flexibility to users to use all the options that http-proxy-middleware provides. Further solution supports multi environments as well. E.g. Dev, Test, Acceptance and Production.

How to use:

Basic:
yarn start

With environment:
yarn start -- --env https://pilot.secret.com.au

Sample configuration (proxy.js):

module.exports = {
  mappings: [{
    url: '/api',
    proxy: true,
    config: {},
  },
    {
      url: '/auth',
      config: {}
    }
  ],
};

Description:

  • Mappings{Array}: One or more proxy mappings
  • proxy{Boolean} (optional): Pipes the requests through computer proxy by reading HTTPS_PROXY or HTTP_PROXY value
  • url{String}: Proxy url (relative or absolute)
  • config{Object}: Accepts any of the valid proxy middleware options

@gaearon
Copy link
Contributor

gaearon commented Jan 14, 2018

  1. proxy string in package.json
  2. proxy object with multiple patterns in package.json
  3. proxy.js file where you can do any routing you want

I would just leave (1) and (3).

(1) is for simple use cases. (3) is for everything else. We've seen so many edge cases I'm not sure there is any value in a "middle" solution. We could provide a few copy-pasteable recipes for common scenarios that used to be solved by (2).

@FezVrasta
Copy link
Contributor

FezVrasta commented Jan 15, 2018

and anyway someone will be able to release a dedicated package to simplify the configuration of the proxy
something like:

// proxy.js
require('react-app-easy-proxy');
// proxy.json
{
 // anything that makes sense
}

@maxhallinan
Copy link

maxhallinan commented Jan 16, 2018

Except for backwards compatibility, a proxy target string in package.json feels incorrect to me.
My understanding is that the use case centers on ease of use. Users would like as little configuration as possible when they just want to set a simple proxy target. That preference could be addressed by supporting a process.env.REACT_APP_PROXY_TARGET variable. create-react-app would generate a default proxy.js file that simply sets this value as the proxy target if it is defined. The advantages are:

  • A single system for proxy configuration. create-react-app becomes a user of its own feature.
  • The most common use case is addressed by a sensible default of that single proxy configuration system.
  • create-react-app no longer provides a reason to pollute package.json with environmental configuration.
  • proxy.js is already there for users who will provide further configuration. Not a huge benefit and perhaps not worth mentioning.

I'm not sure if this makes sense. It would be the ideal for me. Thank you everyone for sharing ideas here. Anything which enables me to configure the proxy via the environment will help me.

@gaearon
Copy link
Contributor

gaearon commented Jan 19, 2018

Except for backwards compatibility, a proxy target string in package.json feels incorrect to me.
My understanding is that the use case centers on ease of use. Users would like as little configuration as possible when they just want to set a simple proxy target. That preference could be addressed by supporting a process.env.REACT_APP_PROXY_TARGET variable

I think this makes sense to me. We introduced proxy in package.json because we didn't have a neat way to specify env variables at the time. But now we support .env files.

@gaearon
Copy link
Contributor

gaearon commented Jan 19, 2018

To everyone: is this sufficient for your use cases?
#3845

@samithaf
Copy link

#3845 This one gives satisfy my requirement.

@snardone
Copy link

This is perfect for us. Thank you.

@yyfearth
Copy link

@iansu @gaearon How about:
Only enable proxy when proxy string in package.json
But allow two cases:

  1. proxy: url, which same as the before
  2. proxy: "proxy.js" or "proxy.json", which will specify the js or json file path for advanced usage.

By this way, the enabling logic is simpler, and does not need to looking for proxy.js/json file
Also user can have the freedom to specify the file names other than proxy.js/json to avoid conflict.

@tiagobnobrega
Copy link

One approach I took a while ago was to customize the start script to allow for "templated" strings, such as :

{
   proxy: "${MY_PROXY_VAR}"
}

the string would be evaluated in the context of process.env. It's very simple for the user to use environment variables for single proxy entry.
I think proxy.js file more elegant and powerful. Maybe it could be a simpler alternative to that.

@doronnac
Copy link

@Timer @gaearon

#3845 looks great to me. My team works with multiple environments so I'll be very happy to see this merged.
Is there anything blocking this?

@iansu
Copy link
Contributor

iansu commented May 10, 2018

@doronnac Yes, there is. Once we get webpack 4 merged into 2.0 we are going to take a look at switching from webpack-dev-server to webpack-serve. At that point #3845 will have to be updated. We're still planning on using the same approach (a proxy.js file in your project) but the implementation will likely change a bit.

@Timer Timer closed this as completed Sep 26, 2018
2.0 automation moved this from Blocker to Ready Sep 26, 2018
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
2.0
  
Ready
Development

No branches or pull requests