Skip to content
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

Express server #4987

Merged
merged 76 commits into from
Mar 12, 2024
Merged

Express server #4987

merged 76 commits into from
Mar 12, 2024

Conversation

ruggi
Copy link
Contributor

@ruggi ruggi commented Mar 4, 2024

Fix #5022

This PR introduces an Express server that will:

  1. Serve the Remix BFF on ports 8000 and 8001
  2. Serve the Haskell server on port 8002
  3. Proxy the relevant calls from the clients to the HS server, so that both the editor and the BFF are available via http://localhost:8000

Additionally, all the missing routes (unless deemed as dead/wontfix/etc.) are added to the Remix routes (or adjusted as needed).

Note that for both the editor and the Remix side of things you'll still have hot reloading available during development (and a very cool-looking ASCII art logo 😎 ).

⚠️ IMPORTANT ⚠️
For local development you'll need to update your env vars to point to the right services. The .env.sample file already contains the right nudges, but what's important is:

...
BACKEND_URL="http://localhost:8002"
...
GITHUB_OAUTH_REDIRECT_URL='http://localhost:8000/v1/github/authentication/finish'
...
EXPRESS_PROXY_TARGET_HOST="localhost"
EXPRESS_PROXY_TARGET_PORT="8002"

@ruggi ruggi marked this pull request as ready for review March 11, 2024 16:46
@ruggi ruggi changed the title Spike/express remix Express server Mar 11, 2024
Comment on lines +100 to +101
host: process.env.EXPRESS_PROXY_TARGET_HOST,
port: process.env.EXPRESS_PROXY_TARGET_PORT,
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note, but we really need to consolidate a bunch of these environment variables that have been introduced for the remix app. Not as part of this PR though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, it'd be great to give this whole thing a cleaning pass

Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be great to give this whole thing a cleaning pass

absolutely a side note, but the best time to give it a cleaning pass is before merging the PR :) the second best time is right after

ruggi and others added 5 commits March 12, 2024 09:22
Co-authored-by: RheeseyB <1044774+Rheeseyb@users.noreply.github.com>
Co-authored-by: RheeseyB <1044774+Rheeseyb@users.noreply.github.com>
Co-authored-by: RheeseyB <1044774+Rheeseyb@users.noreply.github.com>
'Access-Control-Allow-Methods': 'GET,POST,PUT,DELETE,OPTIONS',
'Cache-control': 'no-cache',
})
function defaultResponseHeaders(): Headers {
Copy link
Contributor

Choose a reason for hiding this comment

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

was this change a nicety or a fix for some actual bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, headers are tricky-ish and there are situations where they can end up being duplicated in the browser (and the browser will sometimes scream). Additionally, now proxied raw responses don't need to merge those headers, so I cleaned up contextually

@@ -56,6 +61,7 @@
"@vanilla-extract/dynamic": "2.1.0",
"@vanilla-extract/recipes": "0.5.1",
"@vanilla-extract/sprinkles": "1.6.1",
"chokidar": "^3.6.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

warning, there's a few new unpinned dependencies here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true but they are dev deps and so the CI did not pick it up D: fixing it

\\ \\::/ \\__\\/ \\ \\::/ \\ \\:\\ \\__\\/ \\ \\:\\
\\__\\/ \\__\\/ \\__\\/ \\__\\/
`

Copy link
Contributor

@balazsbajorics balazsbajorics Mar 12, 2024

Choose a reason for hiding this comment

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

Comment on lines +127 to +136
app.use('/authenticate', proxy)
app.use('/editor', proxy)
app.use('/hashed-assets.json', proxy)
app.use('/logout', proxy)
app.use('/p', proxy)
app.use('/project', proxy)
app.use('/share', proxy)
app.use('/sockjs-node', proxy)
app.use('/v1/javascript/packager', proxy)
app.use('/vscode', proxy)
Copy link
Contributor

Choose a reason for hiding this comment

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

are these the endpoints we couldn't express within the Remix routing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nah, we're stretching Remix a bit here I think, and for stuff like the packager or /p we're better off serving them directly from HS (or... in the future directly from Node)

ruggi added 2 commits March 12, 2024 14:28
# Conflicts:
#	utopia-remix/pnpm-lock.yaml
#	utopia-remix/remix.config.js
@ruggi ruggi merged commit f6d564d into master Mar 12, 2024
13 of 15 checks passed
@ruggi ruggi deleted the spike/express-remix branch March 12, 2024 16:52
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.

Express server
3 participants