Skip to content

Conversation

@jdforrester
Copy link
Member

Bug: T293853
Bug: T309772

@santhoshtr
Copy link
Member

If we increase the node version to 18(LTS, Maintainance mode) or later, 'fetch' API is standardized and stable in node. No need for an external library.
(cxserver just migrated away from preq using node native fetch https://phabricator.wikimedia.org/T350773)

@jdforrester
Copy link
Member Author

If we increase the node version to 18(LTS, Maintainance mode) or later, 'fetch' API is standardized and stable in node. No need for an external library. (cxserver just migrated away from preq using node native fetch https://phabricator.wikimedia.org/T350773)

Yes, but other parts of the library aren't yet compatible with Node 18, so we can't do that right now (until I or someone else has the time to fix those incompatibilities).

@mvolz
Copy link
Collaborator

mvolz commented May 27, 2024

Not sure what's causing the errors here, but as I'm looking into preq for an unrelated issue, I thought I should note it differs from fetch in at least two ways:

Preq is wrapping errors, so we might loose some logging features upstream if those are not replaced upstream;

Preq uses another defunct library called requestretry to automatically retry requests once after a short time out.

@jdforrester
Copy link
Member Author

Not sure what's causing the errors here, but as I'm looking into preq for an unrelated issue, I thought I should note it differs from fetch in at least two ways:

Preq is wrapping errors, so we might loose some logging features upstream if those are not replaced upstream;

Preq uses another defunct library called requestretry to automatically retry requests once after a short time out.

Yeah, those two are things to bear in mind for downstream users if they're just preq in prod code, but given this is tests-only here I think it should be fine. But then, it's breaking tests (or something else is), so… :-(

@jdforrester jdforrester changed the title tests: Replace use of preq with node-fetch [WIP] tests: Replace use of preq with native fetch Nov 1, 2024
@mvolz
Copy link
Collaborator

mvolz commented Nov 5, 2024

Weird this works in 20 but not 18 now. Should we just have this be 7.0.0. and drop 18? :)

@jdforrester
Copy link
Member Author

jdforrester commented Nov 7, 2024

Weird this works in 20 but not 18 now. Should we just have this be 7.0.0. and drop 18? :)

Yeah, I'm not quite sure what's going on there – possibly an upstream library version issue? Anyway, let's do that (but in a different PR); most users have bumped by now.

Edit: Proposed in #263 .

@jdforrester jdforrester marked this pull request as draft November 7, 2024 20:09
@jdforrester jdforrester changed the title [WIP] tests: Replace use of preq with native fetch tests: Replace use of preq with native fetch Nov 7, 2024
@mvolz mvolz marked this pull request as ready for review December 12, 2024 13:44
@mvolz mvolz closed this Dec 12, 2024
@mvolz mvolz reopened this Dec 12, 2024
@mvolz mvolz merged commit 2ff83ae into master Dec 12, 2024
2 checks passed
@mvolz mvolz deleted the T293853 branch December 12, 2024 15:58
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.

3 participants