Skip to content

fix: dev server 504 with chunk loding #1611

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

Closed
stormslowly wants to merge 2 commits into from
Closed

fix: dev server 504 with chunk loding #1611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b27e200
fix: 🐛 work around dev 504
stormslowly Sep 24, 2024
b22084f
fix: 🐛 workaround chunk serve back to node.js
stormslowly Sep 24, 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
43 changes: 31 additions & 12 deletions packages/bundler-mako/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,10 @@ exports.dev = async function (opts) {
assert(opts, 'opts should be supplied');
checkConfig(opts);
const express = require('express');
const proxy = require('express-http-proxy');
const app = express();
const port = opts.port || 8000;
const hmrPort = opts.port + 1;
const makoConfig = await getMakoConfig(opts);

// cors
app.use(
Expand Down Expand Up @@ -118,16 +118,36 @@ exports.dev = async function (opts) {
createProxy(opts.config.proxy, app);
}

app.use(
proxy(`http://127.0.0.1:${hmrPort}`, {
filter: function (req, res) {
return req.method == 'GET' || req.method == 'HEAD';
},
skipToNextHandlerFilter: function (proxyRes) {
return proxyRes.statusCode !== 200;
},
}),
);
// serve dist files
app.use(express.static(makoConfig.output.path));
if (process.env.SSU === 'true') {
// for ssu cache chunks

app.use(function (req, res, next) {
if (req.method !== 'GET' && req.method !== 'HEAD') {
return next();
}

let proxy = createProxyMiddleware({
target: `http://127.0.0.1:${hmrPort}`,
selfHandleResponse: true,
onProxyRes: (proxyRes, req, res) => {
if (proxyRes.statusCode !== 200) {
next();
} else {
proxyRes.pipe(res);
}
},
Comment on lines +134 to +140
Copy link

@coderabbitai coderabbitai bot Sep 25, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

需要在 onProxyRes 中设置响应的状态码和头信息

当使用 selfHandleResponse: true 时，需要手动将 proxyRes 的状态码和头信息复制到 res。当前只使用了 proxyRes.pipe(res);，但未设置状态码和头信息，可能导致客户端无法正确解析响应。建议在管道之前，添加以下代码：

+ res.writeHead(proxyRes.statusCode, proxyRes.headers);
  proxyRes.pipe(res);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
onProxyRes: (proxyRes, req, res) => {
if (proxyRes.statusCode !== 200) {
next();
} else {
proxyRes.pipe(res);
}
},
onProxyRes: (proxyRes, req, res) => {
if (proxyRes.statusCode !== 200) {
next();
} else {
res.writeHead(proxyRes.statusCode, proxyRes.headers);
proxyRes.pipe(res);
}
},

onError: (err, req, res) => {
next();
},
Comment on lines +141 to +143
Copy link

@coderabbitai coderabbitai bot Sep 25, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

错误处理时应传递错误对象给 next

在 onError 回调中，应该使用 next(err); 将错误对象传递给 Express 的错误处理中间件，而不是直接调用 next();。这有助于更好地捕获和处理错误信息。

});
Comment on lines +131 to +144
Copy link

@coderabbitai coderabbitai bot Sep 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议将 createProxyMiddleware 的实例移到中间件函数之外以提高性能

当前在每次请求时都会重新创建 proxy 实例，可能会影响性能。建议将 createProxyMiddleware 的实例化移到中间件函数外部，这样只需要创建一次代理，提高效率。


proxy(req, res, () => {
next();
});
});
}

// after middlewares
(opts.afterMiddlewares || []).forEach((m) => {
Expand Down Expand Up @@ -169,7 +189,6 @@ exports.dev = async function (opts) {

// mako dev
const { build } = require('@umijs/mako');
const makoConfig = await getMakoConfig(opts);
if (process.env.HMR === 'none') {
makoConfig.hmr = false;
} else {
Expand Down
1 change: 0 additions & 1 deletion packages/bundler-mako/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
"connect-history-api-fallback": "^2.0.0",
"cors": "^2.8.5",
"express": "^4.18.2",
"express-http-proxy": "^2.1.1",
"get-tsconfig": "4.7.5",
"lodash": "^4.17.21",
"rimraf": "5.0.1",
Expand Down