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

.abort: Don't set headers to default #128

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nicjansma
Copy link

In .abort(), we were setting headers to defaultHeaders. This causes a couple problems.

First, this means you can't set the full User-Agent. Since .open() calls this.abort(), User-Agent will be set after the consumer calls xhr.open(...). If the consumer then calls xhr.setRequestHeader("User-Agent", "foo");, the actual User-Agent would end up as node-XMLHttpRequest, foo instead of just foo because setRequestHeader() appends to the existing value (set by .abort()) if it already exists.

The test-headers.js test actually sets user-agent to node-XMLHttpRequest-test and validates this, but it only works because it sets user-agent and not User-Agent. Since .abort() was just calling headers = defaultHeaders without setting headersCase as well, the test headers ended up as:

{
  "User-Agent": "node-XMLHttpRequest",
  "user-agent": "node-XMLHttpRequest-test"
}

Which would send node-XMLHttpRequest-test over the wire since it was last.

To solve this, I think we can simply not set headers = {} in .abort(), as .send() will set the default headers later anyways.

@nicjansma
Copy link
Author

@driverdan Any chance you have time to review these changes?

@nicjansma
Copy link
Author

@driverdan Are you no longer maintaining this project? It would be great to get feedback so others know whether they can expect their PRs to be answered or not.

@nicjansma
Copy link
Author

@driverdan If you don't have time to review PRs and issues, would you consider adding additional collaborators to help review changes?

@nicjansma
Copy link
Author

Would still like to get this merged in

@hmoog
Copy link

hmoog commented Jun 13, 2018

checkout https://www.npmjs.com/package/xmlhttprequest-ts - i added your pull request

@nicjansma
Copy link
Author

Ping?

@nicjansma
Copy link
Author

Ping...

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