Skip to content

feat(esm): expose User-Agent header #43852

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.

Sign up for GitHub

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

Open
Fyko wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat(esm): expose User-Agent header #43852

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bfc28e9
esm: expose `User-Agent` header
Fyko Jul 15, 2022
b436709
chore: fix test
Fyko Sep 15, 2022
cef0770
test: edit casing for header get
Fyko Sep 18, 2022
75c7015
refactor: standardize lowercase header
Fyko Sep 18, 2022
dc62164
test: finally fix tests
Fyko Sep 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
test: finally fix tests 
The issue was that on L133, the headers object being passed through was overwriting the user-agent header.
I've reformatted the `opts` spread so the only way to over-write the header is to explicitily define it
  • Loading branch information
@Fyko
Fyko committed Sep 18, 2022
commit dc621647d0ec4d0d8e4bc36165674d09dd27fc55
6 changes: 4 additions & 2 deletions lib/internal/modules/esm/fetch_module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,11 @@ function HTTPSGet(url, opts) {
});
return https.get(url, {
agent: HTTPSAgent,
...opts,
headers: {
'user-agent': `Node.js/${process.version}`,
...opts?.headers,
},
Copy link
Member

@jasnell jasnell Aug 28, 2022

Choose a reason for hiding this comment

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

I'm -1 on always setting this. It should be optional and default turned off.

Copy link
Member

@GeoffreyBooth GeoffreyBooth Aug 31, 2022

Choose a reason for hiding this comment

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

How do other server-side runtimes behave?

Copy link
Author

@Fyko Fyko Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

My inspiration for this was how Deno sets the User-Agent header when getting packages. I don't quite understand why so many people are against this -- many very popular packages and utilities do this. Curl sets a header. So does node-fetch. List goes on. And it's completely nonsensical to use some type of env var to set the header.

Copy link
Member

@ljharb ljharb Sep 7, 2022

Choose a reason for hiding this comment

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

“many people do x” isn’t ever a justification; many people do many ill-advised things.

The advantages i can see of setting it by default is easier debugging on the other end - the disadvantages i see is that a server could identify, track, and treat differently a request coming from node vs from other sources.

Copy link
Member

@GeoffreyBooth GeoffreyBooth Sep 7, 2022

Choose a reason for hiding this comment

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

Deno sets the User-Agent header when getting packages. I don’t quite understand why so many people are against this – many very popular packages and utilities do this. Curl sets a header. So does node-fetch.

If most non-browser clients set a User-Agent header, then it’s a de facto standard and we should do the same. It doesn’t matter if some of us disagree with the wisdom of the practice; the point of adding APIs like fetch is to be more standards-compliant and similar to other clients. If users want network requests without User-Agent, they can just override the default or use some other API.

Copy link

@Mifrill Mifrill Nov 27, 2023

Choose a reason for hiding this comment

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

Understanding the concerns about default behavior is essential. While many popular utilities set the User-Agent header by default, there are legitimate concerns regarding potential server-side differentiation based on this header. However, aligning with common practices across other clients could indeed enhance standards compliance.

Perhaps we can consider an optional flag for users to toggle the inclusion of the User-Agent header, offering both flexibility and adherence to commonly accepted practices. This could cater to different user preferences without compromising on standards.

Copy link
Member

@GeoffreyBooth GeoffreyBooth Nov 27, 2023

Choose a reason for hiding this comment

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

Please god not yet another flag. But yes some way to customize this behavior would be good.

...opts,
});
}

Expand All @@ -67,10 +68,11 @@ function HTTPGet(url, opts) {
});
return http.get(url, {
agent: HTTPAgent,
...opts,
headers: {
'user-agent': `Node.js/${process.version}`,
...opts?.headers,
},
...opts,
});
}

Expand Down
2 changes: 2 additions & 0 deletions test/es-module/test-http-imports.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ for (const { protocol, createServer } of [
// ?body sets the body, string
const server = createServer(function(_req, res) {
const url = new URL(_req.url, host);
assert.strictEqual(_req.headers['user-agent'], `Node.js/${process.version}`);

const redirect = url.searchParams.get('redirect');
if (url.pathname === '/not-found') {
res.writeHead(404);
Expand Down
1 change: 0 additions & 1 deletion test/parallel/test-fetch.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ const response = await fetch(`http://localhost:${port}`);
assert(response instanceof Response);
assert.strictEqual(response.status, 200);
assert.strictEqual(response.statusText, 'OK');
assert.strictEqual(response.headers.get('user-agent'), `Node.js/${process.version}`);
const body = await response.text();
assert.strictEqual(body, 'Hello world');

Expand Down