fix: correct code link for alias#2056
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ix/issue-2044 * 'fix/issue-2044' of github.com:eryue0220/npmx.dev: fix: align weekly chart buckets from end to match npm downloads (npmx-dev#2052)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds internal alias import resolution across the codebase. Introduces new types and exports in server/utils/import-resolver.ts (InternalImportTarget, InternalImportsMap, resolveAliasToDir, resolveInternalImport, helpers) and extends createImportResolver to accept an optional internal imports map. Updates server/api/registry/file/[...pkg].get.ts to include Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/unit/server/utils/import-resolver.spec.ts (1)
194-299: Great test expansion; add one#/canonicalisation regression case.Please add a case where the specifier is
#/app/...but the imports key is#app, to lock the slash-variant behaviour.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42e2b622-9ae9-4d7c-b79d-c8d2478854a5
📒 Files selected for processing (4)
server/api/registry/file/[...pkg].get.tsserver/utils/code-highlight.tsserver/utils/import-resolver.tstest/unit/server/utils/import-resolver.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/utils/import-resolver.ts (1)
264-269: Minor performance optimisation: move normalisation outside the loop.
normalizeAliasPrefix(specifier)is computed on every iteration but the result never changes. Consider hoisting it before the loop.♻️ Suggested refactor
function guessInternalImportTarget( imports: InternalImportsMap, specifier: string, files: FileSet, currentFile: string, ): string | null { + const normalizedSpecifier = normalizeAliasPrefix(specifier) for (const [key, value] of Object.entries(imports)) { - const normalizedSpecifier = normalizeAliasPrefix(specifier) const normalizedKey = normalizeAliasPrefix(key)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bda51135-cf70-4e3f-90aa-3fd46fb1d0a8
📒 Files selected for processing (2)
server/api/registry/file/[...pkg].get.tsserver/utils/import-resolver.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/utils/import-resolver.ts (1)
258-316: RefactorguessInternalImportTargetinto smaller helpers.This method currently handles matching, suffix derivation, probing, and return shaping in one block, which makes it harder to maintain and test safely.
As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c7399fb-e4e1-44c9-8cf8-a21c19a00fba
📒 Files selected for processing (1)
server/utils/import-resolver.ts
gameroman
left a comment
There was a problem hiding this comment.
Also when package uses something like this https://main.npmx.dev/package-code/empathic/v/2.0.0/find.mjs#L3 would be nice to correctly resolve that too
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
Updated. |
gameroman
left a comment
There was a problem hiding this comment.
There are some linter warnings but this looks good 👍
ghostdevv
left a comment
There was a problem hiding this comment.
I'm in two minds, one that we shouldn't try and resolve special aliases as they will only work for special packages that are shipping code intended to be processed by something like vite, and the other that we should because it won't hurt - I think the latter is ok as we can always change it later.
Can we also support $ as an alias prefix?
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
🔗 Linked issue
resolve #2044
🧭 Context
support alias path like
~/app/components/Button.vue,@/app/components/Button.vueor#/app/components/Button.vue📚 Description