Add support for websockets in proxy build script

[germanftorres]

Hi,

I have found several issues with the proxy implementation based on got: websockets support was missing and the request body was no being proxied correctly (in POST, PUT...)

This commit adds a dependency on http-proxy which does all the heavy work for proxying requests. I have manually tested POST, GET, and websockets upgrade scenario, but really don't know how to write a proper test.

Please review very carefully.
Thanks!!

[stramel]

I think that adding http-proxy is a step in the right direction and this has a good start. However, I think we're missing lots of power of the lib by limiting it to the string. For instance, my current implementation needs changeOrigin which this doesn't have configurable.

I have a couple ideas that could improve this PR:

• Accept a config object for the value portion of the proxy command that would be passed directly to the proxy instance. This would allow when using a .js config to use the advanced callbacks as well.
• Only load and initialize the proxy for configs that specify a proxy option, so we don't add load time for users not utilizing this feature.

I think we should have @FredKSchott weigh in here before you act on any of my suggestions.

I can also help out with implementation and testing for this feature. It is one of the major things holding me back from being able to port apps over.

Also, wondering if there would be a benefit in offering the basic string configuration for a simple to use for general use-cases?

[fcamblor]

Also, wondering if there would be a benefit in offering the basic string configuration for a simple to use for general use-cases?

For me, this is a must have to keep the basic string config : both in terms of backward compat (even if 2.0 is fresh), simplicity and documentation.

Accept a config object for the value portion of the proxy command that would be passed directly to the proxy instance. This would allow when using a .js config to use the advanced callbacks as well.

💯 with that, something like devOptions.httpProxyOptions and having the exact same representation than http-proxy's options

Currently, my issue regarding content-encoding was fixed only by using http-proxy, but I assume a lot of flexibility should be added to request/response headers using this config.

Only load and initialize the proxy for configs that specify a proxy option, so we don't add load time for users not utilizing this feature.

👍

[stramel]

Also, wondering if there would be a benefit in offering the basic string configuration for a simple to use for general use-cases?

For me, this is a must have to keep the basic string config : both in terms of backward compat (even if 2.0 is fresh), simplicity and documentation.

Agreed, the backwards compat and simplicity I think warrant this feature.

Accept a config object for the value portion of the proxy command that would be passed directly to the proxy instance. This would allow when using a .js config to use the advanced callbacks as well.

💯 with that, something like devOptions.httpProxyOptions and having the exact same representation than http-proxy's options

That's definitely an option that I hadn't thought about. I was thinking something like:

module.exports = {
	scripts: {
	  "proxy:api": {
		  target: ENVIRONMENTS.dev,
		  changeOrigin: true,
		  ws: true,
		}
	}
}

This would allow multiple proxies to be able to be configured in a similar setup to the existing proxying config just with some advanced features/capabilities. In my work setup, we have multiple proxies.

I think we'll also need to create a proxy for each config, unless we move towards a middleware approach like CRA implemented. That I feel is probably out-of-scope though.

[germanftorres]

@stramel @fcamblor
Yes, you're right. Some level of configuration should be available.

I think that the idea behind snowpack is to have a simple directive that uses sensible defaults for the majority of use cases. But, as you point out, there are a lot of edge cases that cannot be expressed in that simple way.

We could add "proxyOptions": {} to the snowpack.config.json to support simple options like changeOrigin. Use of header rewriting and more advanced configurations could be done in the snowpack.config.js though a configuration callback that could receive the proxy instance and the default options.

// snowpack.config.js
module.exports = {
  scripts: {
    'proxy:api': 'proxy http://localhost:5000/api --to /api',
    'proxy:weather': 'proxy http://weather.dev/weather--to /weather'
  },
  proxyOptions: {
    'proxy:weather': (proxy, options) => {
      Object.assign(options, {
        changeOrigin: true
      });
      proxy.on('proxyReq', function (proxyReq, req, res, options) {
        proxyReq.setHeader('X-Special-Proxy-Header', 'foobar');
      });
    },
  },
};

Dont' know if that would be overkill.

[fcamblor]

FYI, created a PR for this PR on @germanftorres repo : germanftorres#1 regarding http-proxy conditional module loading :-)

Regarding config, I would have done something like this :

// snowpack.config.js
module.exports = {
  scripts: {
    'proxy:api': 'proxy http://localhost:5000/api --to /api',
    'proxy:weather': 'proxy http://weather.dev/weather--to /weather'
  },
  devOptions: {
    // General purpose options that are going to be applied on proxy:* configs
    httpProxyOptions: {
        changeOrigin: false
    },
    // Allows finer (maybe overkill) tuning
    configureHttpProxy: (snowpackProxyConfig, httpProxyOptions, proxy) => {
        if(snowpackProxyConfig.id === 'weather') {
            proxy.on('proxyReq', function (proxyReq, req, res, options) {
                proxyReq.setHeader('X-Special-Proxy-Header', 'foobar');
            });
            return Object.assign({}, httpProxyOptions, { changeOrigin: true });
        }
        // For other configs than the `weather` one, keep config as it is
        return httpProxyOptions;
    }
  }
};

My feeling is we're not going to have a lot of different proxy configs, that's why we may have different config levels here :

• Standard (existing, simple) snowpack config based on a single string
• Common config with devOptions.httpProxyOptions flag (if you want to specify some general-purpose http-proxy configs)
• More flexible configuration, defined by code, and aimed at "decorating" proxy instance & options on a per-config basis (config informations passed as parameter ... meaning that if you want to "share" some configuration across multiple configs, that's easy #DRY )

What I like in this is :

• we're not changing the scripts node in snowpack config (this is and will remain a [string, string] map ... I guess that changing this may have a lot of implications)
• DRY principle may be interesting for people wanting to share some proxy configuration across multiple proxy entries.

However, one important thing is : stuff passed to the configureHttpProxy has to be without side effects (for instance, developer should not be able to mute httpProxyOptions, and proxy instance should be bound to the current config : 1 proxy instance per proxy config)

[chiefjester]

@stramel @germanftorres @fcamblor just left some feedback here and here

[fcamblor]

@germanftorres wondering if you shouldn't rename your PR as the http-proxy move has widened a lot the scope of the initial PR :-)

[FredKSchott]

Thanks for the contribution @germanftorres! There's a little bit of cleanup needed to pass linting but instead of going back and forth I'm just going to tackle as a part of merging on the command line to unblock everyone in #371 .