-
Notifications
You must be signed in to change notification settings - Fork 31
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.
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
Fix Studio CI Tests #1836
Fix Studio CI Tests #1836
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
a127aa2
to
6e3d31e
Compare
f07bd65
to
7a2939c
Compare
This reverts commit a6741a4.
It resolves to IPv6 sometimes and breaks local env
Also improve error handling a bit but we're still limited by jest.
NODE_OPTIONS
value
packages/api/src/app-router.ts
Outdated
|
||
app.use((err, _req, res, _next) => { | ||
if (frontend && !_req.path.startsWith(httpPrefix)) { | ||
wwwHandler(_req, res, _next); |
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.
@iameli I'm not sure this works, because I don't know in which scenarios it is used 🙃 Could you double-check?
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.
it's being used to setup a proxy from api server to frontend for livepeer-in-a-box
project.
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.
Will this work after my change? 🙃
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.
should be easy to verify by updating the manifest.yaml
file for that project to your commit in this project. once the build pipeline finishes: https://github.com/livepeer/studio/actions/runs/5938452782/job/16102930628?pr=1836
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.
*branch + commit sha
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.
You probably want to check if err
is null, otherwise this could eat api handler exceptions afaik
It would be weird for this to be called without an error tho as per the express error handler docs
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.
Ok, update to handle err
in the frontend handler.
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.
LGTM either way, especially after confirming if this works when running with the frontend, incl api error responses etc
hopefully it fixes the tests