fix: set cache-control default value to "public, max-age=300" #462
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
follow registry.npmjs.org default value
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| cdnCacheControlHeader: 'max-age=0, s-maxage=120, must-revalidate', | ||
| // if you are using CDN, can override it | ||
| // it meaning cache 300s on CDN server and client side. | ||
| cdnCacheControlHeader: 'public, max-age=300', | ||
| // if you are using CDN, can set it to 'Accept, Accept-Encoding' | ||
| cdnVaryHeader: 'Accept, Accept-Encoding', | ||
| // store full package version manifests data to database table(package_version_manifests), default is false |
There was a problem hiding this comment.
这段代码看起来主要是在配置 Egg 应用，其中涉及到 Http 缓存控制的设置。以下是我的建议：
改进建议：
- cdnCacheControlHeader 可以增加 CLI 配置项，让用户可以根据需要自定义缓存时间，例如 10s 或 1min 等。
- enableCDN 建议改为 enableHttpCache 等更准确的命名。
风险提示：
- 使用 CDN 后一定要谨慎地处理缓存时间的问题，否则可能导致 CDN 缓存错误数据。
| @@ -78,7 +78,7 @@ describe('test/port/controller/PackageBlockController/blockPackage.test.ts', () | |||
| .get(`/${pkg.name}/1.0.0`); | |||
| assert.equal(res.status, 451); | |||
| assert(!res.headers.etag); | |||
| assert(res.headers['cache-control'] === 'max-age=0, s-maxage=120, must-revalidate'); | |||
| assert.equal(res.headers['cache-control'], 'public, max-age=300'); | |||
| assert(res.body.error); | |||
| assert(res.body.error.startsWith('[UNAVAILABLE_FOR_LEGAL_REASONS] @cnpm/testmodule@1.0.0 was blocked, reason: ')); | |||
| assert(res.body.error.includes('only for tests again (operator: cnpmcore_admin/')); | |||
There was a problem hiding this comment.
这段代码是进行测试的，主要测试了访问被屏蔽的模块时返回的响应状态码和相应的Cache-Control头信息是否正确。在第 69 行和第 78 行，将原先的缓存控制信息 max-age=0, s-maxage=120, must-revalidate 更改为了 public, max-age=300 。这个更改看起来合理，并且更符合缓存封装头部的最佳实践。
从代码片段中难以判断是否存在潜在的 bug 风险，还需要查看其他相关的代码才能得出结论。如果有提供更多的信息，我们可以进行更深入的分析并提出进一步的建议。
| assert(res.headers['cache-control'] === 'max-age=0, s-maxage=120, must-revalidate'); | ||
| assert(res.headers.vary === 'Origin, Accept, Accept-Encoding'); | ||
| assert.equal(res.headers['cache-control'], 'public, max-age=300'); | ||
| assert.equal(res.headers.vary, 'Origin, Accept, Accept-Encoding'); | ||
| }); | ||
|
|
||
| it('should 404 when package not exists on abbreviated manifest', async () => { |
There was a problem hiding this comment.
这段代码在做一个包的展示操作，检查了一些返回值和响应头是否符合预期。以下是几点建议：
- 响应头中的缓存控制参数现在设置为了“public, max-age=300”，将
max-age的值从 120 秒延长到 300 秒，缓存时间更长，可以加速用户访问体验。 - 对于响应头中的
vary参数，没有问题，这个参数用于标识缓存请求时需要考虑的额外信息，既然已经针对 Origin、Accept 和 Accept-Encoding 进行了设置，也许不需要再添加其他的标识了。 - 关于代码本身，看起来基本没什么问题，只有一些小的调整需要做，比如规范化响应头中的缓存控制参数以及断言语法。
| assert(res.headers['cache-control'] === 'max-age=0, s-maxage=120, must-revalidate'); | ||
| assert(res.headers.vary === 'Origin, Accept, Accept-Encoding'); | ||
| assert.equal(res.headers['cache-control'], 'public, max-age=300'); | ||
| assert.equal(res.headers.vary, 'Origin, Accept, Accept-Encoding'); | ||
| }); | ||
|
|
||
| it('should latest tag with not scoped package', async () => { |
There was a problem hiding this comment.
这个代码补丁是用来测试包的最新版本和标签。以下是我的简要代码审核：
- 第174行的更改主要是用公共缓存控制头"public, max-age=300"代替了原来的私有“max-age=0, s-maxage=120, must-revalidate”，这有助于提高缓存效率并降低网络流量。
- 第175行的更改使用了断言函数 assert.equal()来确认响应头中"vary"属性的值。
- 从我们的视角来看，此代码没有明显的错误风险，并且可以正常工作。 如果您希望进行改进，建议您考虑实施其他测试用例，以确保完全覆盖所有情况。
Codecov Report
@@ Coverage Diff @@
## master #462 +/- ##
=======================================
Coverage 97.47% 97.47%
=======================================
Files 166 166
Lines 15428 15428
Branches 1996 1996
=======================================
Hits 15038 15038
Misses 390 390
|
fengmk2
pushed a commit
that referenced
this pull request
May 6, 2023
[skip ci] ## [3.20.2](v3.20.1...v3.20.2) (2023-05-06) ### Bug Fixes * set cache-control default value to "public, max-age=300" ([#462](#462)) ([adda725](adda725))
|
🎉 This PR is included in version 3.20.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
follow registry.npmjs.org default value