-
Notifications
You must be signed in to change notification settings - Fork 2
fix(dispatch): upgrade to helix-fetch v2 #398
Conversation
Codecov Report
@@ Coverage Diff @@
## main #398 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 232 242 +10
=========================================
+ Hits 232 242 +10
Continue to review full report at Codecov.
|
}, | ||
"devDependencies": { | ||
"@adobe/eslint-config-helix": "1.1.3", | ||
"@adobe/helix-deploy": "https://github.com/adobe/helix-deploy.git#no-epsagon", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
This PR will trigger a patch release when merged. |
@@ -51,7 +49,7 @@ function getFetchOptions(options) { | |||
delete headers.host; | |||
delete headers.connection; | |||
return { | |||
cache: 'no-cache', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cache option was never supported by helix-fetch (v1 & v2). Using the correct cache: 'no-store'
option causes the following test failure:
1) Index Tests
index produces 404 when fetcher fails due to missing action (with 404 handler).:
TypeError: Stream had error: The operation was aborted.
The same error was also triggered with helix-fetch v1 but was hidden due the incorrect cache option.
Seems like this code is responsible for the failure:
https://github.com/adobe/helix-dispatch/blob/helix-v2/src/index.js#L227-L232
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. this error was due to another problem: for the 404 page, we receive a 200 status, but need to tweak it to look like a 404. with helix-fetch v1, we could just override the response.status
but with v2 this is protected (even worse, the object is not frozen, so no error is thrown, but even if you set res.status=404, it remains 200). so we wrap it with a Proxy
to override the status
. later in the finally
block, it aborts all pending requests, if they are have not produced the response that is returned. since the proxied response is now not the same response again that we check against, it was aborted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I debugged the test with helix-fetch v1 and cache: 'no-store'
. I got the exact same failure (aborted). So it doesn't seem to be related to v2 vs v1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... AFAICR, I added the Proxy
recently, as I couldn't overwrite the status.... but I think it was for the universal deploy changes. this code is so complicated, there might be several layers of potential errors :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we wrap it with a Proxy to override the status
Why not
const { headers, url } = resp;
const resp404 = new Response(res.body, { status: 404, headers, url });
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try... but the problem remains, it's not the same response
as we captured during the fetch. so we don't know if we need to abort or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but with v2 this is protected (even worse, the object is not frozen, so no error is thrown, but even if you set res.status=404, it remains 200)
FWIW: I agree, it is confusing. However it's the same behavior as with the most popular nodejs fetch implementation (node-fetch
, 21m weekly downloads) and in the browser.
related: adobe/helix-home#182 |
now with the recent fixes, it hangs again on openwhisk (and epsagon).... |
looking into it.... |
hmm, just pulled the latest changes and run
No hanging tests. |
yes. but it hangs on openwhisk and espagon enabled... eg:
It's hard to debug - I don't think it's related to helix-fetch, as we had this problems before. but rather with the promise-wars inside helix-dispatch :-) - or a new behaviour of epsagon.... |
using a slightly modified epsagon would resolve the hanging actions: epsagon/epsagon-node#428 |
@tripodsan 3 |
as I said... I'm still waiting for the espagon release... |
## [4.4.8](v4.4.7...v4.4.8) (2021-02-09) ### Bug Fixes * **dispatch:** upgrade to helix-fetch v2 ([#398](#398)) ([ac929cb](ac929cb))
🎉 This PR is included in version 4.4.8 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
No description provided.