jasmine / jasmine-browser-runner

Serve and run your Jasmine specs in a browser
50 stars 25 forks source link

Http proxy support #13

Closed ffortier closed 2 years ago

ffortier commented 2 years ago

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

sgravrock commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Agreed. Closing.