Skip to content

Content base should have precedence over proxy #1132

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

Closed
1 of 3 tasks
jacobp100 opened this issue Oct 3, 2017 · 18 comments
Closed
1 of 3 tasks

Content base should have precedence over proxy #1132

jacobp100 opened this issue Oct 3, 2017 · 18 comments

Comments

@jacobp100
Copy link

jacobp100 commented Oct 3, 2017

  • Operating System: Windows 10
  • Node Version: 8.5
  • NPM Version: 5
  • webpack Version: 3.1
  • webpack-dev-server Version: 2.9.1
  • This is a bug
  • This is a feature request
  • This is a modification request

Code

  // webpack.config.js
{
  contentBase: path.join(process.cwd(), "public"),
  proxy: {
    "/": {
      target: <ourQaServer>
    },
  }
}

Expected Behavior

Content base has priority

Actual Behavior

Proxy has priority

For Bugs; How can we reproduce the behavior?

  • Have file with filename public/style.css
  • Access /style.css
  • It will try (and) fail to get this from the proxy

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

This is confusing behaviour.

This is a repost of #1131. I'm happy to submit a PR if you're happy to change this.

@shellscape
Copy link
Contributor

@jacobp100 I know that unfamiliar systems can be a tad confusing at first. Some helpful tips:

  • when an issue is closed, it can still be commented on, unless it's been locked. your prior issue that was closed as invalid wasn't locked. it's always best to stick to the original issue over reposting a duplicate (we typically close duplicates as well)
  • you can always edit your original post, and that's a better practice than creating a duplicate issue
  • it looks like you may have missed the bit in the Issue Template that reads: Please do remove this header to acknowledge this message.. we curate our templates to be the most useful to both users and our efforts maintaining the project, so please do read them thoroughly

I examined the history behind the proxy feature and how it works with contentBase by examining the git blame for proxy and searching through older issues (they're always there to search, issues are never deleted). The behavior is by design, and unfortunately since you're proxying the root, that will take precedence.

@jacobp100
Copy link
Author

@shellscape I did some searching, but I couldn't find much. I can only find #127, where the order was questioned, although not answered. If its by design, did your examination dig up any history on why that decision was made? Otherwise, I'm not sure I see the use-case for a proxy taking precedence.

I think this is particularly confusing because http-server serves static files in precedence to proxying.

@shellscape
Copy link
Contributor

This goes back 2+ years https://github.com/webpack/webpack-dev-server/blame/50a2e10153c5541fbc03434fcb496e09bc140440/lib/Server.js to when proxy was first implemented, and then to https://github.com/webpack/webpack-dev-server/blame/e6ccbaffc30bdcbc74285d30d1d93c0109019654/lib/Server.js#L336 when the defaultFeatures order was established about a year ago, which resolved #612 in PR #613. Those are the reasons for the established order 🦄

@jacobp100
Copy link
Author

Just trying to understand this. Is the idea that with each proxy config, you can either (but not both):

  • Define a URL to fetch resources from
  • Redirect resources to a later proxy or contentBase

@shellscape
Copy link
Contributor

I think this is particularly confusing because http-server serves static files in precedence to proxying.

If you would be so kind as to paste the links to the docs or a working example that could show this, I think it would be a reasonable change to make for version 3, which is currently in progress. That'd be a breaking change, so it'd have to go into a major version. Version 3 is currently being worked on in the beta branch.

@jacobp100
Copy link
Author

Absolutely! In the docs it reads,

-P or --proxy Proxies all requests which can't be resolved locally to the given url. e.g.: -P http://someurl.com

To me it seems like the current proxy is conflating wanting to redirect files (I.e. a.js to b.js) and reverse proxies.

Would it make sense to pull out the bypass into its own top-level config? We would be free to make contentBase run after bypass but before proxy so we don’t break people’s current use cases.

Happy to submit code for this btw!

@shellscape shellscape reopened this Oct 5, 2017
@shellscape
Copy link
Contributor

shellscape commented Oct 5, 2017

Let's dive a little deeper into this before getting to code. You're linking to a third-party module (http-server) which isn't a core node module, so we can't use that README as an argument for the change because it's not a standard, but just the way that module happens to handle things. That is unless of course you can dig up documentation from https://nodejs.org/en/docs/ that suggests the way that webpack-dev-server has it setup is incorrect. That's the kind of docs we're going to need to justify a big change like this. (express.js can also be considered an authority here, since WDS uses express under the hood to create a server)

@jacobp100
Copy link
Author

I see the confusion—sorry about that! I linked it because it’s a very popular package.

I don’t think node has a built-in proxy or static file server, and express is dependent on how you set up the middleware.

Would it be finding more projects like http-server and getting data on what they do?

Other than that, did my comment about the bypass and proxy splitting make sense?

@shellscape
Copy link
Contributor

Yeah I think we need more documentation to form a non-official but aggregated standard to back up the change. I get what you're saying about bypass and proxy but without a strong case for making those changes it's a tough sell.

@jacobp100
Copy link
Author

jacobp100 commented Oct 9, 2017

I've now gone through quite a lot of pages of http-server-like projects.

A lot of them didn't support proxying. Others did an express-style routes configuration, where you decide the priority of proxies over the static content. I have not included these projects as they don't make a decision on this.

Below is the projects that I found that included proxies and static content, and made a decision about ordering. I've included the downloads per month.

Static files win over proxy
Proxy wins over static files
Unsure

@shellscape
Copy link
Contributor

After spending some time thinking about this, my conclusion is that there isn't a "right way" to go about this ordering of precedence. Either way you go, there are people who will expect it differently. @jacobp100 you're firmly in one camp, perhaps out of necessity, but it's reasonable to say that there will be many others in the other camp.

To that end, we need to devise a meaningful way to adjust the order by which things are positioned in the "features" stack; an array that is put together to assign middleware in a particular order. That'll have to be given some thought as well in order to do that "right."

@shellscape
Copy link
Contributor

While working on the refactor for version 3, I was reminded that one can pass options.features to override the order in which middleware features are executed. see: https://github.com/webpack/webpack-dev-server/blob/master/lib/Server.js#L368

That should provide enough flexibility for the rare circumstances where the ordering needs to be modified. This is an undocumented option, as it's not recommended that consumers play with the ordering, but it looks like it'll work for you in your case.

@twooster
Copy link

This is closed, but I'd like to add my vote for contentBase before proxy. I have a few reasons:

First, this behavior follows the principle of least-surprise and common engineering patterns of trying the fastest possible resolutions before attempting longer ones (local first, remote second). Why should the local webpack build have priority over the proxy if the local contentBase doesn't? Putting the proxy between the two is just wildly inconsistent.

Second, if you use a blanket-proxy -- which would probably be fairly common pattern for proxying to an API backend -- a 404 from the proxied remote will not result in the contentBase being searched for the missing file. The 404 will be displayed to the user. In this case, contentBase has effectively lost all utility. If the contentBase search came first, then it's composable with the proxy, using it as a fallback.

Lastly, some alternatives simply aren't workable. If you try exacting proxy pattern exclusions rather than a blanket pattern, they aren't always possible to express as minimatch patterns. (E.g., I was working with a publicPath of /assets/, but try forming the following pattern so that it works: !/(favicon.ico|robots.txt|assets/**) -- it doesn't work because the ** glob needs to be outside, but if it's outside, it won't match the base files, etc.) Not to mention the maintenance/consistency cost if new files are added.

Anyway, whatever you decide: I found the behavior very surprising and ended up losing hours because -- after seeing that the build overrode the proxy, I assumed the contentBase would as well. At the very least, some documentation is warranted to clarify the ordering/locality mismatch between the build/proxy/contentBase. Or a swap flag. (Or, my favorite, just swapping the order by default. 😁)

@shellscape
Copy link
Contributor

@jacobp100 see previous comment re: options.features. that will provide you the flexibility to order things how you see fit. It's oddly missing from the documentation at https://webpack.js.org/configuration/dev-server/#devserver and is a good opportunity for a contribution.

also important to recognize that we can't cater the logical-assumptions of everyone at once. if this were a hardcoded change, we'd have legions of angry users complaining that it wasn't the previous ordering. that's a seldom-considered condition of being a module used by millions daily (no joke)

@twooster
Copy link

twooster commented Nov 19, 2017

Thanks for the response. 😃

I was being a bit facetious about just swapping the order. Of course that change would aggravate people who depend on the ordering, but you seemed open to the change given the version bump. A feature flag makes more sense if you think the ordering is being used as-is. I understand and am sympathetic that this is a very widely used module.

After thinking through use-cases and staring at the code for a few hours, the plausible use-case I see for proxy in front of contentBase seems to be for doing local path rewrites and giving the local middleware a second chance before falling back to static content. I can see an argument for preserving that behavior. It seems like that feature is getting lumped in with alternate-server proxying -- the use case in exemplified in the documentation.

Can you offer other use-cases that I'm missing?

I spent a while thinking about how to document options.features, too. However since a documented setting is an officially supported setting, I have a couple of concerns with exposing it in its current state. Primarily, the logic behind building the features array is complex and non-obvious (pull #797, #612, amongst others, show the nuance of building this array without introducing errors). I expect that offloading that complexity to users would cause brittle failures down the road.

It's also an odd combination of feature-enablement with middleware ordering (watchConfigBase isn't a middleware, for example, and features are added dynamically rather than handled gracefully if disabled in about half of the handlers, and now there are two places to enable/disable features, which is confusing). I considered making the array constant rather than dynamically generated, and encapsulating the fallback complexity, but accomplishing that with allowing reordering and path rewriting with proxy and historyApiFallback gets... really complex.

I really think a feature flag for this might be the best solution, if you're open. (And leaving features undocumented, basically an internal.)

@twooster
Copy link

(Or, I could submit a pull request just documenting the current behavior and showing a bypass solution?)

@sturman
Copy link

sturman commented Nov 16, 2018

Probably since v3 is released, the link from comment-340639565 is broken. If you use v2 then correct link is https://github.com/webpack/webpack-dev-server/blob/v2/lib/Server.js#L365 or in order to find a source code you may use the keywords const defaultFeatures = in lib/Server.js.

btw, I've solved the issue with the following devServer config:

.....
    devServer : {
        contentBase       : STATIC_ROOT,
        publicPath        : '/',
        host              : 'localhost',
        port              : 3000,
        quiet             : false,
        historyApiFallback: true,
        features          : [
            'before',
            'setup',
            'headers',
            'middleware',
            'contentBaseFiles',
            'proxy',
            'middleware',
            'magicHtml'],
        proxy             : {
            '/': {
                target: 'http://localhost:8080',
                secure: false
            }
        }
    },
.....

@petr-ujezdsky
Copy link

This seems to be fixed in webpack-dev-server v4. I have just tested it on 4.9.1 and the local files take precedence over the proxied ones by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants