Visitar URL original
chore: refine esbuild & node detection by mochaaP · Pull Request #2677 · nodejs/undici · GitHub
Skip to content

chore: refine esbuild & node detection - #2677

Merged
mcollina merged 1 commit into
nodejs:mainfrom
mcha-forks:loader-ua
Feb 9, 2024
Merged

chore: refine esbuild & node detection#2677
mcollina merged 1 commit into
nodejs:mainfrom
mcha-forks:loader-ua

Conversation

@mochaaP

@mochaaP mochaaP commented Jan 31, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

When using the loader for external builtins, esbuildDetection is undefined. This commit defines __UNDICI_IS_NODE__ on globalThis in the loader and deletes it after loading Undici. esbuildDetection has also been extracted as a variable at the top level of the module, to support deleting __UNDICI_IS_NODE__ on globalThis to avoid polluting the global namespace.

Follow up of #2643.

@mochaaP

ghost commented Jan 31, 2024

Copy link
Copy Markdown
Contributor Author

cc @khardix for https://bugzilla.redhat.com/show_bug.cgi?id=2261973

@codecov-commenter

ghost commented Jan 31, 2024 •
edited by codecov-commenter
Loading

Copy link
Copy Markdown

Codecov Report

Attention: 225 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (904bfc2) 85.25%.
Report is 257 commits behind head on main.

Files Patch % Lines
lib/fetch/util.js 56.92% 56 Missing ⚠️
lib/fetch/index.js 68.82% 53 Missing ⚠️
lib/handler/RetryHandler.js 74.35% 30 Missing ⚠️
lib/cache/cache.js 12.12% 29 Missing ⚠️
lib/fetch/dataURL.js 79.31% 12 Missing ⚠️
lib/api/readable.js 83.92% 9 Missing ⚠️
lib/core/diagnostics.js 84.74% 9 Missing ⚠️
lib/eventsource/eventsource.js 96.09% 5 Missing ⚠️
lib/fetch/headers.js 90.38% 5 Missing ⚠️
lib/core/request.js 94.36% 4 Missing ⚠️
... and 6 more
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2677      +/-   ##
==========================================
- Coverage   85.54%   85.25%   -0.30%     
==========================================
  Files          76       84       +8     
  Lines        6858     7589     +731     
==========================================
+ Hits         5867     6470     +603     
- Misses        991     1119     +128

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mochaaP
mochaaP force-pushed the loader-ua branch 3 times, most recently from 93f840b to d4f358a Compare January 31, 2024 08:22
metcoder95
metcoder95 approved these changes Jan 31, 2024
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, but I would like to have another pair of eyes from the team on this

KhafraDev
KhafraDev approved these changes Jan 31, 2024
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(same)

Uzlopak
Uzlopak reviewed Feb 1, 2024
View reviewed changes
Comment thread lib/fetch/index.js Resolved
Uzlopak
Uzlopak reviewed Feb 1, 2024
View reviewed changes
Comment thread lib/fetch/index.js Resolved
Uzlopak
Uzlopak requested changes Feb 1, 2024
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think my proposal is reasonable

Zephyr Lykos
chore: refine esbuild & node detection
904bfc2 
When using the loader for external builtins, `esbuildDetection` is
undefined. This commit defines `__UNDICI_IS_NODE__` on `globalThis`
in the loader and deletes it after loading Undici. `esbuildDetection`
has also been extracted as a variable at the top level of the module,
to support deleting `__UNDICI_IS_NODE__` on `globalThis` to avoid
polluting the global namespace.
@mochaaP
mochaaP requested a review from Uzlopak February 1, 2024 05:48
@Uzlopak

ghost commented Feb 1, 2024

Copy link
Copy Markdown
Contributor

@KhafraDev
@metcoder95

I wonder: In node 21, we have navigator.userAgent. Maybe in case of node, we should check if navigator.userAgent is set and use it, if not fallback to nodejs/${process.versions.node.split('.')[0]} ?

@metcoder95

ghost commented Feb 1, 2024

Copy link
Copy Markdown
Member

I wonder: In node 21, we have navigator.userAgent. Maybe in case of node, we should check if navigator.userAgent is set and use it, if not fallback to nodejs/${process.versions.node.split('.')[0]} ?

Yeah, that SGTM 👍
But maybe even wait for Node v22

@mochaaP

ghost commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

But maybe even wait for Node v22

I agree. We could merge this first and make the UA change in v7, since I consider this a breaking change?

@mcollina

ghost commented Feb 1, 2024

Copy link
Copy Markdown
Member

I would like to avoid breaking changes for Node.js as much as possible, as it would create multiple release lines for us to maintain and backport security fixes.

That's inescapable, so breaking changes will be batched. I'm not sure we would do a undici v7 before Node.js v22 is out, as it mostly depend on the amount of changes we want to ship.

@mochaaP

ghost commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

IMO, it's not that important to change the UA. I couldn't think of possible benefits for changing the user agent, except for collecting metrics for node.js versions.

@Uzlopak

ghost commented Feb 1, 2024

Copy link
Copy Markdown
Contributor

Yeah, lets not open that can of worms.

@mochaaP

ghost commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

Once this lands, we'd be happy to backport this in Fedora.

@KhafraDev

ghost commented Feb 1, 2024

Copy link
Copy Markdown
Member

Not including version numbers in the user-agent was a deliberate change.

mcollina
mcollina approved these changes Feb 1, 2024
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollina

ghost commented Feb 1, 2024

Copy link
Copy Markdown
Member

I'll land this after the security release.

@mochaaP

ghost commented Feb 9, 2024

Copy link
Copy Markdown
Contributor Author

Hi @mcollina, seems this got missed in the security release?

@mcollina
mcollina merged commit 0860735 into nodejs:main Feb 9, 2024
@mcollina

ghost commented Feb 9, 2024

Copy link
Copy Markdown
Member

The goal was to land it after the release.

@metcoder95 metcoder95 mentioned this pull request Feb 14, 2024
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@Uzlopak Uzlopak Uzlopak approved these changes
@metcoder95 metcoder95 metcoder95 approved these changes
@KhafraDev KhafraDev KhafraDev approved these changes
@ronag ronag Awaiting requested review from ronag

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mochaaP @codecov-commenter @Uzlopak @metcoder95 @mcollina @KhafraDev