Remove node-fetch
#11122
Remove node-fetch
#11122
Conversation
| const caPath = process.env.CA_PATH || '/etc/pki/tls/certs/ca-bundle.crt'; | ||
| const certChainPath = | ||
| process.env.CERT_CHAIN_PATH || '/etc/pki/tls/certs/client.crt'; | ||
| const keyPath = process.env.KEY_PATH || '/etc/pki/tls/private/client.key'; | ||
|
|
||
| const [ca, certChain, key] = await loadCerts({ | ||
| caPath, | ||
| certChainPath, | ||
| keyPath, | ||
| }); | ||
|
|
||
| return fetch(url, { | ||
| dispatcher: new Agent({ | ||
| connect: { | ||
| secureContext: createSecureContext({ | ||
| cert: certChain, | ||
| key, | ||
| ca, | ||
| }), | ||
| }, | ||
| }), | ||
| ...options, |
There was a problem hiding this comment.
Is this just a nice refactor or does the new native fetch need this new 'dispatcher' and 'connect' to work ?
There was a problem hiding this comment.
This is a required change as we need to read in the certs async instead of sync. It more closely aligns the pattern we have in our main getCerts file that we use for the application requests.
There was a problem hiding this comment.
Maybe a nitpick but do we need to update the comment on this file that references us using isomorphic-fetch, is the comment still true? Lines 9-11 , they're collapsed so can't comment directly on them 😊
There was a problem hiding this comment.
Good spot, shall remove these!
| @@ -15,7 +13,7 @@ const fetchMarkup = async (url, assetId) => { | |||
| headers: { | |||
| 'User-Agent': 'Simorgh/ws-web-rendering', | |||
| }, | |||
| timeout: SECONDARY_DATA_TIMEOUT, | |||
| signal: AbortSignal.timeout(SECONDARY_DATA_TIMEOUT), | |||
There was a problem hiding this comment.
I haven't seen signal used before ! Why is it preferred over timeout?
There was a problem hiding this comment.
The native version of fetch doesn't have a built-in mechanism for timeout, so we need to use the native AbortSignal functionality provided to do a similar thing for us: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout_static
|
Nice work man! Is there a use case for |
It would need to be an underlying change to the overall fetch mechanism in the BFF, rather than our module specifically since we import the generic |
Overall changes
Removes
node-fetchandisomorphic-fetchin favour of using Node 18+ built-in fetch mechanism, provided byundici. Removes the need to do #9611undicihas been added as a dependency as we need itsAgentconstructor to use our custom certs to authenticate requests.This aligns the Express and Next.js app to use the same fetch mechanism across both apps, and removes the need to keep
node-fetchupdated through its transition to ESM.Code changes
node-fetchandisomorphic-fetchand the instances these libraries were importedgetAgentfunction across the Express and Next.js appsAgentandgetAgentin testsfetchDataFromBFFTesting
yarnto install the new dependenciesyarn devrenderer_env=liveflag to ensure it fetches Live data with the certsHelpful Links
Add Links to useful resources related to this PR if applicable.
Coding Standards
Repository use guidelines