-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat: support unpkg alias path access entry file #674 #675
Conversation
WalkthroughThe recent update enhances the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
114da2e
to
4c92c6a
Compare
* @return | ||
*/ | ||
async #searchPossibleEntries(packageVersion: any, path: string) { | ||
const possiblePath = [ `${path}.js`, `${path}.json`, `${path}/index.js` ]; |
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.
需要考虑 package.json 的 export 么?
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.
我看 unpkg 没考虑?我这里参考 unpkg findEntry 文件的逻辑
async #searchPossibleEntries(packageVersion: any, path: string) { | ||
const possiblePath = [ `${path}.js`, `${path}.json`, `${path}/index.js` ]; | ||
|
||
const possibleFiles = possiblePath.map(pathItem => this.packageVersionFileService.showPackageVersionFile(packageVersion, pathItem)); |
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.
就按 npm package.json 的描述逻辑逐步尝试吧。
有 exports 的优先,
没有则看看 main,
再没有就是 index.js
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.
好像有对应的类库的?
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.
这里可能不是 package 的入口,还可能是 package 下面某个文件夹的下面的文件。
比如:
// 这里实际上会找到 '@opentiny/vue-theme/theme/index.js
import { tinySmbTheme } from '@opentiny/vue-theme/theme'
所以我参考 unpkg findEntry 的逻辑。按顺序尝试匹配 ${path}.js
、${path}.json
、${path}/index.js
、${path}/index.json
写单测,以及本地跑 docker 就可以了? |
单测本地应该没问题,但是测试在 |
7090519
to
a8afe89
Compare
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.
Actionable comments posted: 1
Out of diff range and nitpick comments (2)
app/port/controller/PackageVersionFileController.ts (2)
148-149
: Remove unnecessary blank line.The blank line between the variable declaration and the following code is unnecessary and can be removed for better readability.
151-152
: Remove unnecessary blank line.The blank line between the
if
statement and the following code is unnecessary and can be removed for better readability.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/port/controller/PackageVersionFileController.ts (2 hunks)
- test/fixtures/@cnpm/test-find-entry/package.json (1 hunks)
- test/port/controller/PackageVersionFileController/listFiles.test.ts (1 hunks)
Files not reviewed due to errors (1)
- test/port/controller/PackageVersionFileController/listFiles.test.ts (no review received)
Files skipped from review due to trivial changes (1)
- test/fixtures/@cnpm/test-find-entry/package.json
Additional comments not posted (1)
app/port/controller/PackageVersionFileController.ts (1)
184-195
: Ensure all possible paths are covered.Verify that the list of possible paths covers all common cases. If there are additional common cases, consider adding them to the list.
a8afe89
to
72bd7d5
Compare
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/port/controller/PackageVersionFileController.ts (2 hunks)
- test/fixtures/@cnpm/test-find-entry/package.json (1 hunks)
- test/port/controller/PackageVersionFileController/listFiles.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- app/port/controller/PackageVersionFileController.ts
- test/fixtures/@cnpm/test-find-entry/package.json
- test/port/controller/PackageVersionFileController/listFiles.test.ts
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.
LGTM
@@ -352,5 +352,41 @@ describe('test/port/controller/PackageVersionFileController/listFiles.test.ts', | |||
assert.equal(called, 1); | |||
assert.equal(resList.filter(res => res.status === 409 && res.body.error === '[CONFLICT] Package version file sync is currently in progress. Please try again later.').length, 1); | |||
}); | |||
it('should redirect to possible entry', async () => { |
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.
单测挂了,看看
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.
我再看看
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.
我想请问下 cnpmcore 对 redis 版本有要求吗?我用的 redis-server 5.0.7,在 window 和 wsl 下都有上面的报错。看起来像是 redis 版本有问题?
@fengmk2
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.
https://github.com/cnpm/cnpmcore/blob/master/docs/setup.md 安装的是最新版本 redis server
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.
确实是版本的问题,现在修复了。麻烦大佬再点一下同意跑跑GitHub的单测。
@fengmk2
… into feat/unpkgAliasPath
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/port/controller/PackageVersionFileController/listFiles.test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- test/port/controller/PackageVersionFileController/listFiles.test.ts
closes #674
Summary by CodeRabbit
New Features
Tests