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

Adds a setConfig option to hoxy #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adds a setConfig option to hoxy #89

wants to merge 1 commit into from

Conversation

trueadm
Copy link

@trueadm trueadm commented Nov 23, 2016

By allowing the user to setConfig, they can customise the request validProtocols, removeHeaders and nonEntityMethods with custom values.

Note: tests do not pass for me (they also fail on master), am I doing something wrong to get them working?

By allowing the user to setConfig, they can customise the request validProtocols, removeHeaders and nonEntityMethods with custom values.
@greim
Copy link
Owner

greim commented Nov 26, 2016

Thanks for taking the time to do a PR. I'm concerned since these changes would force the same configuration on every running instance of a proxy, it could create bugs in some scenarios. For example two areas in a codebase that don't really "know" about each other, trying to set config on hoxy and conflicts arising. To the extent these things should be customizable, they should be customizable per-instance, i.e. by passing options to createServer().

var proxy = hoxy.createServer({
  ...
  validProtocols: { ... },
  removeHeaders: { ... },
  nonEntityMethods: { ... },
});

I realize it's a deeper change, but I think it would be the right thing to do.

@greim
Copy link
Owner

greim commented Nov 26, 2016

Also, regarding tests, I haven't run them on more recent versions of Node. Have you verified the test failures are happening against only your branch, or against master also?

Never mind, I see you mentioned that they do fail on master.

@greim
Copy link
Owner

greim commented Nov 27, 2016

Just pushed an update that fixed the test failures on my end. Let me know if you still run into issues.

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.

2 participants