fix: devServer put static serve proxy after umi proxy middleware #1558
Conversation
# Conflicts: # packages/bundler-mako/index.js
|
Warning Rate limit exceeded@stormslowly has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 46 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Walkthrough此次更改涉及多个文件，主要集中在开发服务器的请求处理、测试功能的增强以及配置文件的引入。具体而言，开发服务器的静态内容处理逻辑得到了改进，测试模块增加了对服务器状态的检查功能，同时新增了环境配置文件、样式文件和异步测试函数，以支持开发环境的灵活性和可维护性。 Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (5)
e2e/fixtures.umi/dev.hmr/.umirc.ts (1)
1-14: 建议添加注释以提高可读性。为了提高代码的可读性和可维护性，建议为每个配置项添加简短的注释，解释其用途和影响。
建议添加如下注释：
export default { + // 禁用 MFSU（Module Federation Speed Up） mfsu: false, + // Mako 配置（暂时为空） mako: {}, + // Less 加载器配置 lessLoader: { + // 始终计算数学表达式 math: 'always', }, + // 代理配置 proxy: { '/api': { + // 目标服务器地址 'target': 'http://jsonplaceholder.typicode.com/', + // 修改请求头中的 host 值 'changeOrigin': true, + // 重写路径 'pathRewrite': { '^/api' : '' }, } } };e2e/fixtures.umi/dev.hmr/expect.mjs (1)
1-37: 测试套件结构良好，但可以进一步改进。这个测试套件为不同类型的资源提供了良好的基本覆盖。然而，我们可以通过以下方式使其更加全面和结构化：
使用测试框架：考虑使用像 Jest 这样的测试框架，它提供更好的结构化和报告功能。
分组测试：将相关的测试分组到描述性的块中，例如：
describe('UmiJS 开发服务器测试', () => { test('首页应正确响应', async () => { // fetchHome 逻辑 }); test('API 应返回正确的 JSON', async () => { // fetchApi 逻辑 }); // 其他测试... });
添加更多边缘情况：测试无效路由、大文件、并发请求等。
环境设置：添加 beforeAll 和 afterAll 钩子来启动和关闭服务器。
配置测试：允许通过环境变量或配置文件更改端口等设置。
这些改进将使测试套件更加健壮和可维护。
scripts/test-e2e.mjs (2)
6-18:checkServer函数实现良好
checkServer函数的实现是合理的，它正确地使用了net模块来检查服务器连接状态。建议考虑添加一个超时机制，以防止在某些情况下连接挂起太长时间：
function checkServer(port, host, callback) { const client = new net.Socket(); + const timeout = 5000; // 5秒超时 + + const timer = setTimeout(() => { + client.destroy(); + callback(false); + }, timeout); client.connect({ port, host }, () => { + clearTimeout(timer); client.end(); callback(true); }); client.on('error', () => { + clearTimeout(timer); client.destroy(); callback(false); }); }
Line range hint
1-131: 总体评价：显著改进，但仍有优化空间这次更改大大增强了
scripts/test-e2e.mjs文件的功能和灵活性。主要改进包括：
- 新增了服务器连接检查功能。
- 增加了处理不同类型测试（dev 和 build）的能力。
- 改进了期望（expect）文件的导入和执行逻辑。
这些改进使得测试脚本更加健壮和通用。然而，仍有一些优化空间：
- 错误处理：在整个脚本中增加更详细的错误处理和日志记录。
- 代码结构：将一些重复的逻辑抽取为独立的函数，以提高可维护性。
- 配置管理：考虑将硬编码的值（如端口号、超时时间等）移至配置文件。
- 测试覆盖：为新增的函数（如
checkServer、waitForServer）添加单元测试。总的来说，这些更改是积极的，显著提升了脚本的功能。通过实施上述建议，可以进一步提高代码质量和可维护性。
crates/mako/src/dev.rs (1)
203-207: 改进的静态文件服务日志修改后的调试日志消息现在包含了正在服务的文件路径，这提供了更多上下文信息，有助于跟踪哪些静态文件正在被服务。
为了使日志消息更加清晰，建议稍微调整一下格式：
debug!("Serving static file: {}", path);这样可以更明确地表示正在执行的操作。
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (9)
- crates/mako/src/dev.rs (3 hunks)
- crates/mako/src/visitors/env_replacer.rs (1 hunks)
- e2e/fixtures.umi/dev.hmr/.env (1 hunks)
- e2e/fixtures.umi/dev.hmr/.umirc.ts (1 hunks)
- e2e/fixtures.umi/dev.hmr/expect.mjs (1 hunks)
- e2e/fixtures.umi/dev.hmr/pages/index.less (1 hunks)
- e2e/fixtures.umi/dev.hmr/pages/index.tsx (1 hunks)
- packages/bundler-mako/index.js (2 hunks)
- scripts/test-e2e.mjs (4 hunks)
Files skipped from review due to trivial changes (4)
- crates/mako/src/visitors/env_replacer.rs
- e2e/fixtures.umi/dev.hmr/.env
- e2e/fixtures.umi/dev.hmr/pages/index.less
- e2e/fixtures.umi/dev.hmr/pages/index.tsx
Additional comments not posted (6)
e2e/fixtures.umi/dev.hmr/.umirc.ts (2)
1-14: 配置文件结构正确，符合UmiJS标准。整体配置结构看起来符合UmiJS的标准，包含了MFSU、Mako、Less加载器和代理设置。
7-13: 验证代理配置的正确性和安全性。代理配置将'/api'路由转发到外部服务。请确保这是预期的行为，并验证目标服务器的安全性。
运行以下脚本以验证代理配置：
e2e/fixtures.umi/dev.hmr/expect.mjs (1)
1-4: 导入语句和端口定义看起来不错。导入语句和端口定义适合此测试文件的目的。
scripts/test-e2e.mjs (1)
1-1: 导入 'net' 模块是合适的新增的 'net' 模块导入是必要的，用于实现服务器连接检查功能。
crates/mako/src/dev.rs (1)
124-125: 新增的调试日志有助于请求跟踪这个新增的调试日志语句可以记录传入请求的 HTTP 方法和 URI 路径，这对于调试和监控服务器行为非常有帮助。
packages/bundler-mako/index.js (1)
149-176: 新增中间件以优化请求处理逻辑这个新增的中间件主要用于处理非HTML内容的GET和HEAD请求，通过创建代理到本地开发服务器来优化请求处理流程。
优点：
- 简化了请求处理逻辑，移除了之前的
processReqURL函数。- 通过检查请求方法和内容类型，更精确地控制了代理行为。
- 引入了错误处理机制，提高了系统的健壮性。
建议：
- 考虑将代理创建逻辑抽取为一个单独的函数，以提高代码的可读性和可维护性。
- 确保这个更改不会影响到其他类型的请求处理，特别是HTML请求的处理逻辑。
+ function createDevServerProxy(hmrPort) { + return createProxyMiddleware({ + target: `http://127.0.0.1:${hmrPort}`, + selfHandleResponse: true, + onProxyRes: (proxyRes, req, res) => { + if (proxyRes.statusCode !== 200) { + next(); + } else { + proxyRes.pipe(res); + } + }, + onError: (err, req, res) => { + next(); + }, + }); + } app.use((req, res, next) => { if (req.method !== 'GET' && req.method !== 'HEAD') { return next(); } if (req.accepts().join('').includes('html')) { return next(); } - const devServerProxy = createProxyMiddleware({ - target: `http://127.0.0.1:${hmrPort}`, - selfHandleResponse: true, - onProxyRes: (proxyRes, req, res) => { - if (proxyRes.statusCode !== 200) { - next(); - } else { - proxyRes.pipe(res); - } - }, - onError: (err, req, res) => { - next(); - }, - }); + const devServerProxy = createDevServerProxy(hmrPort); devServerProxy(req, res, () => { next(); }); });请运行以下脚本以验证HTML请求的处理是否受到影响：
whyer11
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1558 +/- ##
=======================================
Coverage 62.11% 62.11%
=======================================
Files 129 129
Lines 15559 15559
=======================================
Hits 9665 9665
Misses 5894 5894 ☔ View full report in Codecov by Sentry. |
|
上一个版本想复用wsProxy，但是qiankun这种请求子应用的方式是使用XHR，bundler中无法判断这个请求是要请求css还是html，所以转发到mako devServer后404了。 |
…ijs_proxy # Conflicts: # crates/mako/src/visitors/env_replacer.rs # scripts/test-e2e.mjs
stormslowly
force-pushed
the
fix/umijs_proxy
branch
from
a330d90
to
1a41fc6
Compare
stormslowly
changed the title
stormslowly
changed the title
stormslowly
force-pushed
the
fix/umijs_proxy
branch
2 times, most recently
from
a8596ff
to
332315b
Compare
stormslowly
force-pushed
the
fix/umijs_proxy
branch
from
332315b
to
d19d66d
Compare
stormslowly
force-pushed
the
fix/umijs_proxy
branch
from
c41a0b5
to
9d6bdec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/bundler-mako/index.js (1)
121-130: 代码看起来不错，有一点小建议。这段代码通过
express-http-proxy中间件将请求直接代理到本地 HMR 服务器，并根据请求方法（GET 或 HEAD）进行过滤。对于代理响应状态码不是 200 的情况，会跳过当前处理程序。总体而言，这是一个合理的实现。不过为了更好的可观测性，建议对代理响应状态码不是 200 的情况进行日志记录，例如：
app.use( proxy(`http://127.0.0.1:${hmrPort}`, { filter: function (req, res) { return req.method == 'GET' || req.method == 'HEAD'; }, skipToNextHandlerFilter: function (proxyRes) { + if (proxyRes.statusCode !== 200) { + console.log(`Proxy response status code: ${proxyRes.statusCode}`); + } return proxyRes.statusCode !== 200; }, }), );
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (7)
- crates/mako/src/dev.rs (3 hunks)
- e2e/fixtures.umi/dev.hmr/.env (1 hunks)
- e2e/fixtures.umi/dev.hmr/expect.js (1 hunks)
- e2e/fixtures.umi/dev.hmr/global.css (1 hunks)
- packages/bundler-mako/index.js (2 hunks)
- packages/bundler-mako/package.json (1 hunks)
- scripts/test-e2e.mjs (2 hunks)
Files skipped from review due to trivial changes (2)
- e2e/fixtures.umi/dev.hmr/.env
- e2e/fixtures.umi/dev.hmr/global.css
Files skipped from review as they are similar to previous changes (3)
- crates/mako/src/dev.rs
- e2e/fixtures.umi/dev.hmr/expect.js
- scripts/test-e2e.mjs
Additional comments not posted (2)
packages/bundler-mako/package.json (1)
12-12: 添加依赖项express-http-proxy看起来不错！引入
express-http-proxy包可以增强应用程序通过 Express 处理 HTTP 代理功能的能力。这个包可以方便地将请求路由到不同的服务器或服务，从而提高应用程序管理 API 调用或充当代理请求中间件的能力。版本约束^2.1.1允许进行次要版本和补丁更新，确保与应用程序的兼容性。packages/bundler-mako/index.js (1)
85-85: 看起来不错！引入
express-http-proxy模块作为proxy变量，为后续的请求代理做准备。
stormslowly
force-pushed
the
fix/umijs_proxy
branch
from
9d6bdec
to
b804d6b
Compare
stormslowly
changed the title
stormslowly
changed the title
Fix the issue where the devServer does not correctly return HTML type resources, and add related e2e test cases for umijs devServer.
Summary by CodeRabbit
新功能
错误修复