webpack-dev-server icon indicating copy to clipboard operation
webpack-dev-server copied to clipboard

devServer proxy /api/* is actually /api

Open olivercoad opened this issue 5 years ago • 22 comments

  • Operating System:
  • Node Version:
  • NPM Version:
  • webpack Version:
  • webpack-dev-server Version: 3.10.3
  • Browser:
  • [x] This is a bug
  • [ ] This is a modification request

Code

When using the 'context': 'target' or 'context': { ...options... } style of proxy, if /* is at the end of the context it is removed.

https://github.com/webpack/webpack-dev-server/blob/50c09a4b64c013cca0acb6013bdaa28d0f342149/lib/Server.js#L230-L233

See discussion in #359 for why this was done

So, for a config such as this:

// webpack.config.js
module.exports = {
  //...
  devServer: {
    proxy: {
      '/api/*' : 'http://localhost:3000'
    }
  }
};

Expected Behavior

Proxies everything starting with /api/
i.e. /api/stuff but NOT /apistuff.html

Actual Behavior

Proxies everything starting with /api
including /apistuff.html

For Bugs; How can we reproduce the behavior?

  1. Create new project using The SAFE Template: dotnet new SAFE
  2. To make it clear when requests are being proxied to the server, replace use_static publicPath with use_router (text "from server") in src/Server/Server.fs
  3. Run the project using fake build -t run
  4. Observe that localhost:8080/apistuff.html results in from server, because it is being proxied to the server (whereas localhost:8080/notapistuff.html results in Cannot GET /notapistuff.html because it is not being proxied)

For Features; What is the motivation and/or use-case for the feature?

N/A

olivercoad avatar Mar 10 '20 11:03 olivercoad

Bug, thanks for the issue, feel free to send a PR

alexander-akait avatar Mar 10 '20 11:03 alexander-akait

Happy to make a PR. I've got a couple different thoughts of how to approach fixing it.

I think the most important part about this issue that the behaviour is different between the two styles of proxy config.

It would be quite reasonable to expect that changing the config from

devServer: {
    proxy: {
        '/api/*' : 'http://localhost:3000'
    }
}

to

devServer: {
    proxy: [
        {
            context: '/api/*',
            target: 'http://localhost:3000'
        }
    ]
}

would not have an effect on the behaviour.

However, because the latter does not modify the proxyOptions before sending it HPM, it actually has the behaviour of proxying ONLY /api/*, and NOT for example /api or /api/v1/stuff.

Just not altering the context is not really an option because it would break backward compatibility (as seen in #359)

With that in mind, what do you think of the following options.

Instead of removing trailing /*, replace it with /**

This would result in the expected behaviour,

Proxies everything starting with /api/ i.e. /api/stuff but NOT /apistuff.html

but would not fix the discrepency between the two styles and so changing to the array style would still have an unexpected change

Deprecate the use of trailing '/*'

We may as well fix the discrepency with using a single '*' while we're at it. Deprecate the use of '*' and traling '/*' in the 'context': 'target' style in favour of '**' and '/**'

Deprecate the use of the 'context': 'target' style altogether

Supporting only arrays and not properties would be a simplification of the code base.

Configurations that need extra config need to use the { ...options...} style anyway so may as well use { context: '/api', ...options... }

And for simple proxies that do not need extra configuration, we could add support for https://github.com/chimurai/http-proxy-middleware#shorthand

olivercoad avatar Mar 10 '20 11:03 olivercoad

I think we should use logic as in express

alexander-akait avatar Mar 10 '20 11:03 alexander-akait

Hey guys, is there an update on this? I'm having trouble proxying across federated modules (used to work before beta v4)

vtrikoupis avatar Apr 01 '21 11:04 vtrikoupis

Not sure. I we can decide which suggested option to take (or something else) I'd still be happy to make a PR.

@vtrikoupis I'd suggest changing any trailing /* with /** or using the array style.

olivercoad avatar Apr 01 '21 14:04 olivercoad

I think we can remove it for the next major release (beta now)

alexander-akait avatar Apr 01 '21 14:04 alexander-akait

What do you mean by "remove it"? Remove what?

olivercoad avatar Apr 01 '21 15:04 olivercoad

backwards compatibility for old package

alexander-akait avatar Apr 01 '21 16:04 alexander-akait

So what exactly are you saying? Just simply remove these lines?

https://github.com/webpack/webpack-dev-server/blob/feb7eb044eb0b2cfe7b58d19ea9c35dd049053d0/lib/Server.js#L174-L175

That would be a breaking change and likely cause issues with a lot of people's configurations.

olivercoad avatar Apr 01 '21 16:04 olivercoad

I think we can remove it for the next major release (beta now)

alexander-akait avatar Apr 01 '21 16:04 alexander-akait

Yes I understand doing it in a major release but not everyone is going to read the release notes or understand the ramifications.

olivercoad avatar Apr 01 '21 16:04 olivercoad

In this case we don't solve this problem, on the one hand we cannot do it, on the other we have to fix it

alexander-akait avatar Apr 01 '21 16:04 alexander-akait

@alexander-akait have you considered the suggestions in https://github.com/webpack/webpack-dev-server/issues/2454#issuecomment-597044551 ?

olivercoad avatar Apr 01 '21 17:04 olivercoad

hm, make sense, I think we should add deprecation message here, so in future we will drop it

alexander-akait avatar Apr 01 '21 17:04 alexander-akait

Feel free to send a PR

alexander-akait avatar Apr 01 '21 17:04 alexander-akait

Okay. Just to be clear, do you mean deprecating use of * and trailing /*, or deprecating the 'context': 'target' style altogether?

olivercoad avatar Apr 01 '21 17:04 olivercoad

Ideally we should be as close as possible to http-proxy-middleware package, I think we should support this:

{
  proxy: [
    {
      context: Filter | Options
      options?: options?: Options
    }
  ]
}

From node_modules/http-proxy-middleware/dist/index.d.ts

alexander-akait avatar Apr 01 '21 17:04 alexander-akait

We really have a lot of complex stuff here...

alexander-akait avatar Apr 01 '21 17:04 alexander-akait

I don't remember all cases (because I don't write them :smile: ), so we need be careful, but I agree, we need simplify it and remove unnecessary complex

alexander-akait avatar Apr 01 '21 17:04 alexander-akait

Ideally we should as close as possible to http-proxy-middleware package, I think we should support this:

{
  proxy: [
    {
      context: Filter | Options
      options?: options?: Options
    }
  ]
}

From node_modules/http-proxy-middleware/dist/index.d.ts

Agreed. So it sounds like you reckon deprecate the object synax like

{
   proxy: { '/api': 'http://localhost:3000' }
}

in favour of the full array syntax like http-proxy-middleware.

olivercoad avatar Apr 01 '21 17:04 olivercoad

Yep, ideally we should avoid using object syntax, full array syntax like http-proxy-middleware is more flexibility and avoid misleading between options dev server and http-proxy-middleware package

alexander-akait avatar Apr 01 '21 17:04 alexander-akait

Great. Will make a PR

olivercoad avatar Apr 01 '21 17:04 olivercoad

Fixed in 5.0.0

alexander-akait avatar Feb 12 '24 13:02 alexander-akait