fix: handle scoped npm package aliases SvelteSource.resolveAlias#112
fix: handle scoped npm package aliases SvelteSource.resolveAlias#112LucHeart wants to merge 2 commits into
Conversation
WalkthroughThis pull request introduces two changes. First, a Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/build.yml (1)
9-9: Update comment to reflect the new trigger.The comment now needs updating to mention the
workflow_dispatchtrigger added below.📝 Suggested documentation update
-# The workflow is triggered on push and pull_request events. +# The workflow is triggered on push, pull_request, and manual workflow_dispatch events.src/main/kotlin/com/github/warningimhack3r/intellijshadcnplugin/backend/sources/impl/SvelteSource.kt (1)
87-96: Please add regression coverage for alias-prefix selection.This block now needs to preserve
$lib/..., single-segment@..., and scoped@scope/pkg/...resolution across both exact and wildcardpathsentries. A focused test matrix here would make this kind of parsing change much safer.Based on learnings: In SvelteSource alias resolution, SvelteKit uses both exact alias keys (e.g., "$lib") and wildcard keys (e.g., "$lib/*"), so the current lookup approach handles both patterns correctly. The path values in tsconfig are consistently arrays, not strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36c711b2-e725-49af-8b71-eac6e6ebcd82
📒 Files selected for processing (2)
.github/workflows/build.ymlsrc/main/kotlin/com/github/warningimhack3r/intellijshadcnplugin/backend/sources/impl/SvelteSource.kt
| val (aliasPrefix, suffix) = if (alias.startsWith("@")) { | ||
| val parts = alias.split("/", limit = 3) | ||
| val prefix = if (parts.size >= 2) "${parts[0]}/${parts[1]}" else alias | ||
| val rest = if (parts.size >= 3) parts[2] else "" | ||
| prefix to rest | ||
| } else { | ||
| alias.substringBefore("/") to alias.substringAfter("/", "") | ||
| } | ||
|
|
||
| val paths = parseTsConfig(tsConfig) | ||
| .asJsonObject?.get("compilerOptions") | ||
| ?.asJsonObject?.get("paths") | ||
| ?.asJsonObject?.get(alias.substringBefore("/")) | ||
|
|
||
| val aliasPath = (paths?.asJsonObject?.get(aliasPrefix) | ||
| ?: paths?.asJsonObject?.get("$aliasPrefix/*")) | ||
| ?.asJsonArray?.get(0) | ||
| ?.asJsonPrimitive?.content ?: throw Exception("Cannot find alias $alias in $tsConfig") | ||
| val normalized = aliasPath.replace(Regex("^\\.+/"), "") | ||
| val suffix = alias.substringAfter("/", "") | ||
| val normalized = aliasPath.replace(Regex("^\\.+/"), "").removeSuffix("/*") | ||
| val resolved = if (suffix.isEmpty()) normalized else "$normalized/$suffix" |
There was a problem hiding this comment.
This regresses single-segment @ aliases.
The new branch treats every @... import as @scope/package/.... That breaks previously supported aliases like @/components/ui/button and @components/foo, because the resolver now looks for @/components/* or @components/foo/* instead of falling back to the original single-segment key (@/*, @components/*, etc.).
🛠️ One way to preserve both scoped and non-scoped forms
- val (aliasPrefix, suffix) = if (alias.startsWith("@")) {
- val parts = alias.split("/", limit = 3)
- val prefix = if (parts.size >= 2) "${parts[0]}/${parts[1]}" else alias
- val rest = if (parts.size >= 3) parts[2] else ""
- prefix to rest
- } else {
- alias.substringBefore("/") to alias.substringAfter("/", "")
- }
+ val defaultCandidate = alias.substringBefore("/") to alias.substringAfter("/", "")
+ val scopedCandidate = alias
+ .takeIf { it.startsWith("@") }
+ ?.split("/", limit = 3)
+ ?.takeIf { it.size >= 2 && it[0] != "@" }
+ ?.let { "${it[0]}/${it[1]}" to it.getOrElse(2) { "" } }
val paths = parseTsConfig(tsConfig)
.asJsonObject?.get("compilerOptions")
?.asJsonObject?.get("paths")
- val aliasPath = (paths?.asJsonObject?.get(aliasPrefix)
- ?: paths?.asJsonObject?.get("$aliasPrefix/*"))
- ?.asJsonArray?.get(0)
- ?.asJsonPrimitive?.content ?: throw Exception("Cannot find alias $alias in $tsConfig")
- val normalized = aliasPath.replace(Regex("^\\.+/"), "").removeSuffix("/*")
- val resolved = if (suffix.isEmpty()) normalized else "$normalized/$suffix"
+ fun resolveCandidate(aliasPrefix: String, suffix: String): String? {
+ val aliasPath = (paths?.asJsonObject?.get(aliasPrefix)
+ ?: paths?.asJsonObject?.get("$aliasPrefix/*"))
+ ?.asJsonArray?.get(0)
+ ?.asJsonPrimitive?.content ?: return null
+
+ val normalized = aliasPath.replace(Regex("^\\.+/"), "").removeSuffix("/*")
+ return if (suffix.isEmpty()) normalized else "$normalized/$suffix"
+ }
+
+ val resolved = (scopedCandidate?.let { (aliasPrefix, suffix) ->
+ resolveCandidate(aliasPrefix, suffix)
+ } ?: resolveCandidate(defaultCandidate.first, defaultCandidate.second))
+ ?: throw Exception("Cannot find alias $alias in $tsConfig")
WarningImHack3r
left a comment
WarningImHack3r
left a comment
There was a problem hiding this comment.
Thanks a LOT for your contribution! Could you also bring that to other implementations once we're fixed on this one?
| # Trigger the workflow on any pull request | ||
| pull_request: | ||
| # Allow manual trigger | ||
| workflow_dispatch: |
There was a problem hiding this comment.
I'm not particularly a fan of that as the use cases are really rare, but it doesn't hurt either I guess
| val aliasPath = parseTsConfig(tsConfig) | ||
| // Handle scoped package aliases (e.g. @repo/ui/components). | ||
| // For @-prefixed aliases, the package name includes a slash (@scope/name), | ||
| // so we split into at most 3 parts to correctly separate the prefix from the rest. |
There was a problem hiding this comment.
At first glance, splitting with exact numbers feels wonky, CodeRabbit review seems right on the kind of problem that could arise.
But to be honest, I'm not really sure what's the best way to handle this: maybe looking at what the original shadcn implementation looks like could help?
2 participants
When using something like turborepo in my case, you end up with scoped paths like @repo/yourPackageName/components/
The previous path resolver always cut this of at the first slash, which usually works if you have something like $lib/components/
We now handle scoped namespaces separate,
Not sure if that is also an issue with other frameworks, but this solves it for Svelte.
PS: The workflow change is kinda forced, since I needed to push again after enabling actions to trigger a CI build.
So I was like, why not add a dispatch trigger while am at it, hope you dont mind.