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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Http proxy support #13

Closed
wants to merge 3 commits into from
Closed

Conversation

ffortier
Copy link

@ffortier ffortier commented Feb 8, 2022

Add a proxy configuration using http-proxy-middleware so that we can stub paths and avoid errors in the console.

@sgravrock
Copy link
Member

This seems like a reasonable feature to have, and the code looks fine to me. But it increases the set of dependencies pretty significantly for a feature that most users aren't likely to use. I wonder if we'd be better off providing a way to let users add arbitrary middleware, e.g.:

// spec/support/jasmine-browser.js
const { createProxyMiddleware } = require('http-proxy-middleware');

module.exports = {
  middleware: {
    '/cdn': createProxyMiddleware({"target": "http://localhost:8889"})
  },
  "srcDir": "src",
  "srcFiles": ["**/*.js"],
  // ...
};

That way users who want proxy support can have it, and those who don't want it won't get the extra dependencies.

On the other hand, I haven't really thought through the implications for maintainability of letting user code add arbitrary middleware.

cc: @slackersoft

@slackersoft
Copy link
Member

I think going the middleware route seems like the best option here. If we do that, I think the API we want here would allow the internals to turn each key-value-pair in the middleware object into a call to app.use() (https://github.com/jasmine/jasmine-browser-runner/blob/main/lib/server.js#L115) after doing a collision check for paths that we are already mapping. This means we'd be just relying on the middleware being implemented so it could just be passed along to Express.

@sgravrock
Copy link
Member

Agreed. Closing.

@sgravrock sgravrock closed this Sep 3, 2022
sgravrock added a commit that referenced this pull request Aug 12, 2023
* Supports uses like serving extra static assets, proxying requests to
  another server, etc.
* Fixes #37
* Fixes #38
* See #13
* See #33
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

Successfully merging this pull request may close these issues.

None yet

3 participants