Skip to content

Conversation

@hidde-jan
Copy link
Contributor

@hidde-jan hidde-jan commented Feb 14, 2025

Currently the proxy for proxying graphql requests is implemented as a page. On my machine, this causes intermittent 500 errors, see guacsec/guac#2494

Next offers native proxy capabilities. Using that capability solves the intermittent issues for me. It also seems like it improves performance.

The remaining issue in this PR is that the query URL should come from src/config.ts, but this would require converting the next config to typescript.

next.config.js Outdated
return [
{
source: "/api/graphql",
destination: "http://localhost:8080/query",
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if the GUAC GQL server is running somewhere else? Should we be checking for the value of GUACGQL_SERVER_URL here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wait. I haven't had coffee yet. This is probably what you meant by "The remaining issue in this PR is that the query URL should come from src/config.ts, but this would require converting the next config to typescript." 😄

So assuming we merge this now, what's the failure case when the server is running somewhere else? It seems like src/config.ts already checks for the GUACGQL_SERVER_URL environment variable, so we should be able to lean on that, I think. I'm not very familiar with Javascript and friends, I'm more of a sysadmin type, so forgive me if I'm missing something obvious.

@hidde-jan
Copy link
Contributor Author

hidde-jan commented Feb 15, 2025 via email

@hidde-jan
Copy link
Contributor Author

After some digging, it turns out that v14 of next does not support TS config files. To keep the URLs in the central config, I needed to convert config.ts to config.js. I've also placed the proxy path in the same file, so there's a single source of truth for that.

@funnelfiasco
Copy link
Collaborator

Trying to test it locally, I'm getting this error:

 ○ Compiling / ...
 ⨯ node:console
Module build failed: UnhandledSchemeError: Reading from "node:console" is not handled by plugins (Unhandled scheme).
Webpack supports "data:" and "file:" URIs by default.
You may need an additional plugin to handle "node:" URIs.
Import trace for requested module:
node:console
./src/config.js
./apollo/client.ts
./app/page.tsx

I'm not a JavaScript person, so this could be user error, but I haven't been able to find a solution.

@hidde-jan
Copy link
Contributor Author

hidde-jan commented Feb 20, 2025 via email

Signed-off-by: Hidde-Jan Jongsma <[email protected]>
Copy link
Collaborator

@funnelfiasco funnelfiasco left a comment

Choose a reason for hiding this comment

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

LGTM. I don't have write access on this repo, so it doesn't "count", but I'll ask the maintainers to take a look. Thanks again @hidde-jan!

Copy link
Contributor

@mlieberman85 mlieberman85 left a comment

Choose a reason for hiding this comment

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

LGTM

@mlieberman85 mlieberman85 merged commit a10cb4b into guacsec:main Mar 6, 2025
2 checks passed
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