Skip to content
This repository has been archived by the owner on Jun 7, 2019. It is now read-only.

Disable user-agent header when client options were not provided - Closes #696 #712

Merged
merged 3 commits into from
Jul 17, 2018

Conversation

yatki
Copy link
Contributor

@yatki yatki commented Jul 11, 2018

What was the problem?

lisk-elements is throwing Refused to set unsafe header “User-Agent”warning when it's used in browsers.

How did I fix it?

I updated getUserAgent function to return empty object when clientOptions were not provided.

How to test it?

should not set User-Agent header when client options were not given

Review checklist

@yatki yatki self-assigned this Jul 11, 2018
@yatki yatki requested a review from willclarktech July 11, 2018 10:29
@yatki yatki changed the title ♻️ disable user-agent header when client options not provided Disable user-agent header when client options were not provided - Closes #696 Jul 11, 2018
@willclarktech
Copy link
Contributor

@yatki This is failing on CI.

Copy link
Contributor

@willclarktech willclarktech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Two minor requests.

{
'User-Agent': getUserAgent(options.client),
},
getUserAgent(options.client),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Object.assign call combines various headers objects. User agent is on the wrong semantic level for this. The function as written doesn't just get the user agent, it gets a headers object which happens to only include the user agent (currently). We should either use the previous pattern or rename the function something like getClientHeaders.


it('should not set User-Agent header when client options were not given', () => {
apiClient = new APIClient(defaultNodes, {
version: customHeaders.version,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually version/customHeaders.version doesn't make sense here. Could you remove it from the other test too?

@willclarktech willclarktech merged commit ba2ba6c into 1.1.0 Jul 17, 2018
@willclarktech willclarktech deleted the 696-disable-user-agent-in-browser branch July 17, 2018 13:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants