-
Notifications
You must be signed in to change notification settings - Fork 174
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
Express server #4987
Conversation
# Conflicts: # utopia-remix/app/components/projectActionContextMenu.tsx # utopia-remix/app/hooks/useFetcherWithOperation.tsx # utopia-remix/app/routes/projects.tsx # utopia-remix/app/store.tsx # utopia-remix/app/types.ts
host: process.env.EXPRESS_PROXY_TARGET_HOST, | ||
port: process.env.EXPRESS_PROXY_TARGET_PORT, |
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.
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
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 agree, it'd be great to give this whole thing a cleaning pass
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'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
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 { |
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.
was this change a nicety or a fix for some actual bug?
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.
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
utopia-remix/package.json
Outdated
@@ -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", |
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.
warning, there's a few new unpinned dependencies here!
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.
true but they are dev deps and so the CI did not pick it up D: fixing it
\\ \\::/ \\__\\/ \\ \\::/ \\ \\:\\ \\__\\/ \\ \\:\\ | ||
\\__\\/ \\__\\/ \\__\\/ \\__\\/ | ||
` | ||
|
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.
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) |
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.
are these the endpoints we couldn't express within the Remix routing?
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.
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)
# Conflicts: # utopia-remix/pnpm-lock.yaml # utopia-remix/remix.config.js
Fix #5022
This PR introduces an Express server that will:
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 😎 ).
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: