Snowpack: Proxy Configuration

[FredKSchott]

Spin off from #371
/cc @fcamblor @stramel @germanftorres and anyone else who's interested

How do we support configuring the proxy?

Proposal 1

Support a non-string script.

scripts: {
  "proxy:api": {
    url: 'http://localhost:5000/api',
    to: '/api',
    target: ENVIRONMENTS.dev,
    changeOrigin: true,
    ws: true,
  }
}

Proposal 2

{
  scripts: {
    'proxy:api': 'proxy http://localhost:5000/api --to /api',
  },
  proxyOptions: {
    'proxy:api': (proxy, options) => { },
    // or:
    'proxy:api': { ... },
  },
};

Proposal 3

{
  scripts: {
    'proxy:api': 'proxy http://localhost:5000/api --to /api --config api-proxy-config.js',
  }
};
// Not pictured: a api-proxy-config.js file that exports the proper config

Other Proposals?

Feel free to brainstorm more in the comments

[FredKSchott]

Proposal 2 is my favorite for a couple of reasons. Since proxying is built-into Snowpack, it makes sense to give it its own top-level config support. This also works well for a snowpack.config.js config file, where you could define your proxy configuration inline in the config object or outside of it in a separate file (or top of the config file) if it gets too long/verbose.

Honestly I may even like it more than our current "scripts" system, which is starting to feel limiting. Instead of splitting your proxying configuration across "scripts" and "proxyConfig", we could just support:

  scripts: {},
  proxy: {
    '/api': 'http://localhost:5000/api',
    '/api2': { ... }
  },

I guess that would be a Proposal 4 then. Curious to hear thoughts.

[fcamblor]

@FredKSchott Not sure if you captured my comment here into the Proposal #3 (what is api-proxy-config.js's format ? the same format than http-proxy options ? Or a snowpack-specific (very close to the http-proxy) one ?)

I'm not really fan of providing potentially multiple programmatic configuration points, I'd rather provide a single configuration point, passing the current proxy as input parameter (that way, if developer wants to add specificities to some snowpack configs he can ... and if he wants to share proxy config across multiple snowpack config he can easily as well)

My opinion about Proposals :

• Proposal 1 : will break Map<string, string> scripts node configuration (not a big deal, but it may have a lot of Typescript impacts here and there to take the new Map<string, string|ProxyConfig> into consideration)
• Proposal 2 : I find it a bit verbose, having to maintain same keys (config ids) across multiple nodes doesn't look very convenient (but OTOH, we have separation of concerns here)
• Proposal 3 : Why not, if this is a snowpack-specific file format that allows to take snowpack configs as input parameters
• Proposal 4 : It fixes a lot of the downsides, but doesn't provide ease to share the same configuration across multiple proxy configs

[stramel]

If we're good with breaking away from scripts, I would be more in favor of Proposal 4.

I previously was sticking with the scripts to make it function similarly to how scripts do currently. You can even maintain the DRY principle using any of these proposals when using a snowpack.config.js file.

const options = {
  target: ENVIRONMENTS.dev,
  changeOrigin: true,
  ws: true,
}

module.exports = {
  scripts: {
    'proxy:api': 'proxy http://localhost:5000/api --to /api',
    'proxy:api1': {
      path: '/proxy',
      ...options
    },
    'proxy:api2': {
      path: '/proxy2',
      ...options
    }
  }
}

My preference is

1. 4
2. 1
3. 2

I don't think Proposal 3 would be overly useful since you're already in a config file.

My major concern with Proposal 4 is how it would be implemented for customization other than basic string:string mapping.

[fcamblor]

You can even maintain the DRY principle using any of these proposals when using a snowpack.config.js file.

I agree if it comes to properties configuration only.
But if you think about having to register proxy handlers like :

proxy.on('proxyReq', function (proxyReq, req, res, options) {
    proxyReq.setHeader('X-Special-Proxy-Header', 'foobar');
});

It can become complicated to put some parts of the handler common and some others specifics, without having the snowpack id config in hands.

But to be 100% honest, we could use function factories like :

const createOptionsFor = (name, proxy) => {
  proxy.on('proxyReq', function (proxyReq, req, res, options) {
      proxyReq.setHeader('X-Special-Proxy-Header', "foobar");
      proxyReq.setHeader('X-Proxified-Backend', name==='api1'?"coming-from-api1":"coming-from-api2");
  });
  return {
    target: ENVIRONMENTS.dev,
    changeOrigin: name === 'api1',
    ws: true,
  };
};

module.exports = {
  scripts: {
    'proxy:api': 'proxy http://localhost:5000/api --to /api',
    'proxy:api1': (proxy, options) => ({
      path: '/proxy',
      ...createOptionsFor('api1', proxy)
    }),
    'proxy:api2': (proxy, options) => ({
      path: '/proxy2',
      ...createOptionsFor('api2', proxy)
    })
  }
}

[FredKSchott]

Yea, I think supporting code reuse would be a useful goal. Maybe we focus only on the object use-case for now, and leave a "function configuration" off for a future use-case. I actually think every usecase that we know about today is met by just configuring the proper options. Also, I'm not sure what the options argument would be in that case :)

[FredKSchott]

I think supporting anything other than a string in the "scripts" config is out of scope, for now. We may do it one day, but the whole pitch is that they're super linear, simple strings. I'd hate to break that design goal so soon after v2.0 launch. I think that kills proposal 1, unfortunately.

Regardless, for backwards compatibility we should deprecate but still support the simple proxy script usage that we do now. But then if you need more advanced configuration, use use the "proxy" or "proxyOptions" configuration object.

[thisguychris]

Hi guys! 👋 Chiming in, my vote on 4. It's a configuration that mostly straightforward. Also, don't want to complicate things since we just merge it, but do we plan to use http-proxy as a hard dependency for a foreseeable future?

At work we just use fetch as proxy like this

async function handleRequest(request) {
  return await fetch(URL, request)
}

And it proxies everything PUT, POST, DELETE, etc. So if we plan to use http-proxy as a hard dependency then tailing the options against it's API make sense.

Regardless, for backwards compatibility we should deprecate but still support the simple proxy script usage that we do now. But then if you need more advanced configuration, use use the "proxy" or "proxyOptions" configuration object.

Yeah, if we have something like that under the hood, then it's best we expose it.

[stramel]

At work we just use fetch as proxy like this

async function handleRequest(request) {
  return await fetch(URL, request)
}

And it proxies everything PUT, POST, DELETE, etc. So if we plan to use http-proxy as a hard dependency then tailing the options against it's API make sense.

This was previously how it was handled which had many shortcomings especially when it comes to customization of the proxied request and proxying websockets.

[thisguychris]

At work we just use fetch as proxy like this

async function handleRequest(request) {
  return await fetch(URL, request)
}

And it proxies everything PUT, POST, DELETE, etc. So if we plan to use http-proxy as a hard dependency then tailing the options against it's API make sense.

This was previously how it was handled which had many shortcomings especially when it comes to customization of the proxied request and proxying websockets.

I think we were using got, but yeah, I just mentioned this since at work, we are using native fetch, not npm module.

[FredKSchott]

Yea, I think http-proxy is meant to iron over a bunch of issues with got & fetch, although I don't personally use this much myself so I'm going off the experiences of others.

Okay, so I'm feeling good about Proposal 4 also. To flesh it out a bit more:

// snowpack.config.js
const BASE_CONFIG = {};

module.exports = {
  "scripts": {},
  "proxy": {
    // Simple usecase, similar to `proxy` script support today
    '/api': 'http://localhost:5000/api',
    // Advanced usecase
    '/api2': { ... },
    // Supports config reuse
    '/api3': { ...BASE_CONFIG, ... },
  }
}

@stramel given that you're the one with the use-case that you can test against, any interest in implementing the above? It actually shouldn't be too hard, and I'm happy to help along the way.

[stramel]

Sure, I can take a stab at it! This should resolve a major blocker for me to switch a work project over to this.

Are we still supporting the scripts string:string mapping as well?

[FredKSchott]

Awesome! I think we should keep supporting the existing "proxy:*" scripts usecase, and normalize it to a new "proxy" config during the "normalizeConfig()" step so that we don't need to support two different kinds of config in the rest of the codebase.

For implementing, the only thing that I'm aware of that may need to change is that I don't know if we can keep using 1 proxy sever instance for all proxied requests. We may need to create multiple http-proxy instances, one with each config value, and then pass the config in there. Currently, we pass the config directly to the proxy.web() function. I might be wrong though, but just something to look out for.

[stramel]

I think we should keep supporting the existing "proxy:*" scripts usecase, and normalize it to a new "proxy" config during the "normalizeConfig()" step so that we don't need to support two different kinds of config in the rest of the codebase.

That's what I was thinking too.

For implementing, the only thing that I'm aware of that may need to change is that I don't know if we can keep using 1 proxy sever instance for all proxied requests. We may need to create multiple http-proxy instances, one with each config value, and then pass the config in there. Currently, we pass the config directly to the proxy.web() function. I might be wrong though, but just something to look out for.

Agreed. I'm not sure either but I will be sure to try each approach and do a bit of research on it.

[stramel]

I got it roughly working with a single proxy instance. Successfully proxying requests and WebSockets. I have it converting scripts to the new format. I still have a bunch of clean up to get it ready for a PR.

The proxy startup is averaging 12ms startup time when using a proxy option/script and 0ms for people not proxying. So no real big load added.

A few questions for @FredKSchott:

• This syntax seems backwards to me "proxy http://SOME.URL --to /PATH" since my request is against /PATH to http://SOME.URL. Was this intentional?
• By default, http-proxy has ignorePath set to false. However, how the current proxy scripts work, would be like setting ignorePath to true. How would you like to handle this?
• What option name do you have in mind? proxy/proxies/proxyOptions?
• Do I need to support CLI args for advanced proxying?
• How extensively do I need to validate the config schema for the proxy options?
• How would you like to handle output including errors? There can be a lot of output from WebSocket proxying.

[FredKSchott]

That's acceptable 👍, we could even look to deferring this to run post-startup (or first proxied request) but honestly 12ms is fine by me for now.

To your questions:

• proxy format was meant to match mount format, that's really it :) Since we're moving this out of the script object entirely there's no need to keep supporting this syntax (besides backwards compatibility with the old script format)
• I'm not sure if I understand ignorePath enough to comment, but it sounds like we're doing this work already so that we can set it to true?
• I like proxy best
• No, I think this can be config only, i don't think we need to support any CLI flags related to proxying.
• No validation needed on our end, as long as we're properly reporting back errors that http-proxy would surface.
• What do you mean by output, do you mean console logging?

Thanks for tackling this, looking forward to getting this merged/released!

[stramel]

That's acceptable 👍, we could even look to deferring this to run post-startup (or first proxied request) but honestly 12ms is fine by me for now.

Oh, that's a good idea. I think I can pull that off.

• proxy format was meant to match mount format, that's really it :) Since we're moving this out of the script object entirely there's no need to keep supporting this syntax (besides backwards compatibility with the old script format)

Okay, so I will make sure this part is backwards compatible.

I'm not sure if I understand ignorePath enough to comment, but it sounds like we're doing this work already so that we can set it to true?

So currently, "proxy https://pokeapi.co/api/v2 --to /api" proxies "/api/pokemon/ditto" -> "https://pokeapi.co/api/v2/pokemon/ditto". This is using ignorePath: true. However, the default functionality of http-proxy is ignorePath: false. Which in the current state would be "proxy https://pokeapi.co --to /api" and result in "/api/pokemon/ditto" -> "https://pokeapi.co/api/pokemon/ditto". I'm tempted to lean towards the defaults of the library so we don't have to document a custom default.

I like proxy best

No, I think this can be config only, i don't think we need to support any CLI flags related to proxying.

👍

No validation needed on our end, as long as we're properly reporting back errors that http-proxy would surface.

This was related to the config json schema validation. Sorry for not making that clear. Does that change your response?

What do you mean by output, do you mean console logging?

yes the console logging. Previously it output something like proxy:api...............[DONE] proxy https://pokeapi.co/api/v2 --to /api. I can do something similar but not show all the options. I'm more concerned about where to output all the connect/disconnect logs of WebSockets and errors.

[FredKSchott]

Okay, so I will make sure this part is backwards compatible.

Just to make sure nothing got lost in translation, the new short-form should just be the full URL that we resolve to: "/api": "http://localhost:3000/api/foo"

No validation needed on our end, as long as we're properly reporting back errors that http-proxy would surface.

This was related to the config json schema validation. Sorry for not making that clear. Does that change your response?

Yup, that's fine. Just saying it's a string (shorthand) or object (long-form) would be enough.

What do you mean by output, do you mean console logging?

yes the console logging.

Yea, lets just remove the proxy:api........... line item from the dashboard, it never made sense it the "build" step anyway so I actually think this might be a good call on its own.

Lets let it do it's default output for now, and then revisit it if its too noisy. We actually do an okay job of clearing the console regularly, so we should only need to address this if we see it become an actual problem.

[FredKSchott]

I'm not sure if I understand ignorePath enough to comment, but it sounds like we're doing this work already so that we can set it to true?

Okay, that makes sense. I do like having it be for the short-form ignorePath: true so that you can be more expressive, aka "/api": "http://localhost:3000/api/foo". I think we'd have just as much trouble documenting the ignorePath:false behavior ("no, you actually can't do that")

(there's a chance I'm still mixing the two true/false behaviors up, in which case ignore me :)

For the long-form, we should just literally pass in the exact object that they send us.

[germanftorres]

Sorry for being late to the party and thanks for all the feedback to my first PR ever.

I really like the simple approach of a simple config string, be it in scripts or in proxyOptions and wouldn't complicate configuration too much.

The main use case for me is to be able to split the javascript app from the backend (/api) during development to take advantage of snowpacks superfast reload experience. In production, we serve both the js and the backend them from the same domain.

In this context, I would stick to the simple string format and try to cover the most useful options for development out of the box:

• mount like syntax
• websockets support (oob or enabled by --ws flag in the syntax)
• ssl dev support (oob secure: false + changeOrigin: true or switchable through flag).

Advantages: simple to configure, doesn't leak any http-proxy implementation details into snowpack's configuration and covers a simple dev use case.

Great to see a lot of excitement around this project!!

[FredKSchott]

@germanftorres the current "proxy:api" script syntax will be supported for the rest of Snowpack v2's life, so that's not going away anytime soon.

But, proxy: {"/api": "http://localhost:8081/api"} feels almost identical to the current script syntax, so I feel like you should be well supported there as well for basic usage. Plus, then you can easily convert to the proposed proxy object format for ws, secure, changeOrigin, etc.