Skip to content

support backend hmr #277

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.

Sign up for GitHub

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

Merged
benjreinhart merged 4 commits into from
Sep 16, 2024
Merged

support backend hmr #277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
06eb1dd
support server hmr
swk777 Sep 12, 2024
7fb1e75
add lock file
swk777 Sep 12, 2024
5c63653
remove console
swk777 Sep 12, 2024
ef3c33d
add comment
swk777 Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/api/dev-server.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,14 @@ process.on('SIGINT', async function () {
server.close();
process.exit();
});
if (import.meta.hot) {
import.meta.hot.on('vite:beforeFullReload', () => {
wss.close();
server.close();
});

import.meta.hot.dispose(() => {
wss.close();
server.close();
});
}
2 changes: 1 addition & 1 deletion packages/api/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"dist/**"
],
"scripts": {
"dev": "vite-node dev-server.mts",
"dev": "vite-node -w dev-server.mts",
"test": "vitest",
"prebuild": "rm -rf ./dist",
"build": "tsc && cp -R ./drizzle ./dist/drizzle && cp -R ./srcbook/examples ./dist/srcbook/examples && cp -R ./prompts ./dist/prompts",
Expand Down
2 changes: 1 addition & 1 deletion packages/api/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"noEmit": false,
"allowImportingTsExtensions": false,
"outDir": "dist",
"types": ["vitest/globals"]
"types": ["vitest/globals", "vite/client"]
},
"include": ["**/*.mts"],
"exclude": ["node_modules", "dist"]
Expand Down
2 changes: 1 addition & 1 deletion packages/api/tsconfig.lint.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"include": ["**/*.mts", "vite.config.ts", "drizzle.config.ts"],
"include": ["**/*.mts", "vite.config.ts", "drizzle.config.ts", "vite-env.d.ts"],
"exclude": ["node_modules", "dist"]
}
1 change: 1 addition & 0 deletions packages/api/vite-env.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/// <reference types="vite/client" />
2 changes: 1 addition & 1 deletion packages/configs/ts/base.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"isolatedModules": true,
"useDefineForClassFields": true,
"lib": ["es2022", "DOM", "DOM.Iterable"],
"module": "Preserve",
"module": "esnext",

@benjreinhart benjreinhart Sep 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you explain this change? cc @versecafe

@versecafe versecafe Sep 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@benjreinhart there shouldn't be a huge difference between esnext and preserve but removing preserve allows for some small transformations in tsc for cleaning things up, this should be a fine change and even make things easier but does mean a test to make sure the full bundled version is resolving correctly needs to be done to prevent a mishap like the 0.0.1 release bug we had

@benjreinhart benjreinhart Sep 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sweet, so long as it's all working with no major downsides LGTM

@swk777 swk777 Sep 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you @versecafe for the confirmation. I've discovered that "module: preserve" doesn't work well with vite-node watch mode. If necessary, we can consider keeping "module: esnext" specifically for the development mode @benjreinhart

"moduleDetection": "force",
"moduleResolution": "bundler",
"noUncheckedIndexedAccess": true,
Expand Down
1 change: 1 addition & 0 deletions packages/web/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"@types/react-dom": "^18.3.0",
"@vitejs/plugin-react-swc": "^3.7.0",
"autoprefixer": "^10.4.20",
"chokidar": "^3.6.0",
"postcss": "^8.4.45",
"tailwindcss": "^3.4.11",
"vite": "^5.4.4"
Expand Down
21 changes: 20 additions & 1 deletion packages/web/vite.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,29 @@
import path from 'path';
import { defineConfig } from 'vite';
import react from '@vitejs/plugin-react-swc';
import chokidar from 'chokidar';

// https://vitejs.dev/config/
export default defineConfig({
plugins: [react()],
plugins: [
react(),
// reload on backend change
{

@benjreinhart benjreinhart Sep 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need this on the client, to reload the websocket connection?

@versecafe versecafe Sep 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should just be reloading the ws on the client whenever the API rebuilds in dev mode which is needed otherwise it's connected to a nonexistent session and breaks but @swk777 correct me if i'm wrong on that

@versecafe versecafe Sep 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should just be reloading the ws on the client whenever the API rebuilds in dev mode which is needed otherwise it's connected to a nonexistent session and breaks but @swk777 correct me if i'm wrong on that

@benjreinhart benjreinhart Sep 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's what I assume. I would be ok with this as a stop-gap but the better solution here is for the client side websocket client to re-establish connections when the connection is closed. Robust websocket clients use a combination of heartbeats and onclose listeners to attempt reconnect with a backoff strategy.

For now I'm ok to merge this but I would like to comment and/or file an issue to capture this.

@benjreinhart benjreinhart Sep 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is already some retry logic inside of the client, but I think this is retrying to send a message if it fails, so maybe it doesn't handle this case properly.

https://github.com/srcbookdev/srcbook/blob/main/packages/web/src/clients/websocket/client.ts#L10-L11

@benjreinhart benjreinhart Sep 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#280

@swk777 swk777 Sep 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@versecafe is right, and as @benjreinhart mentioned, this approach is indeed better. I’ll work on implementing this later.
Another point that deserves attention is that in the current design, the cellId is randomly generated, which changes after a reload. As a result, cell operations cannot be continued seamlessly across hmr without reload.

@benjreinhart benjreinhart Sep 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's a good point, we need to fix those.

name: 'backend-watch-reload',
configureServer(server) {
const watcher = chokidar.watch('../api/**/*.mts', {
ignored: /node_modules/,
persistent: true,
});
watcher.on('change', () => {
server.ws.send({
type: 'full-reload',
});
});
},
},
],

resolve: {
alias: {
'@': path.resolve(__dirname, './src'),
Expand Down
4 changes: 3 additions & 1 deletion pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.