Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
mary-ext
force-pushed
the
feat/teardown
branch
from
ebff3f8 to
b8e2989
Compare
| '/impact/**': { | ||
| headers: { | ||
| 'Cross-Origin-Opener-Policy': 'same-origin', | ||
| 'Cross-Origin-Embedder-Policy': 'require-corp', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I am not sure what the ideal approach is here, setting the COOP/COEP headers here means that it is not possible to transition to /impact/ route directly without a full page navigation
mary-ext
force-pushed
the
feat/teardown
branch
from
b8e2989 to
ff9f5f8
Compare
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
mary-ext
force-pushed
the
feat/teardown
branch
from
157c953 to
8b1663c
Compare
|
blocked on rolldown/rolldown#8145 for Safari support |
Which Safari version are you using? People upstream think you need to use latest version to fix the rolldown issue 🤔 |
hrm I must've caught a different issue then, someone reported that https://teardown.kelinci.dev/ didn't work on iOS Safari 26. I don't have an Apple device so the only way I could test it is with GNOME Web, since that runs WebKitGTK and should've been pretty close to Safari (even then 'pretty close' is an understatement because Safari runs on a closed source fork of WebKit) |
|
in any case, since it doesn't block Safari on macOS and iOS I believe this PR is mostly ready to go aside from figuring out how we should be adding COOP/COEP/CORP headers, if we just have it in I can try rebasing this branch later during the weekend |
mary-ext
force-pushed
the
feat/teardown
branch
from
0ed5d0c to
b8f55be
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
mary-ext
force-pushed
the
feat/teardown
branch
from
b8f55be to
9e59752
Compare
mary-ext
force-pushed
the
feat/teardown
branch
from
9e59752 to
a76f9ea
Compare
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive bundle impact analysis feature to the application. It adds a complete bundler system with package resolution, dependency hoisting, tarball fetching, and AST-based module analysis, utilising a dedicated Web Worker for processing. The bundler computes bundle metrics including gzip and Brotli compression sizes, generates chunks, and provides detailed export information. A new user-facing page and components display bundle analysis results, including subpath selection, export filtering, and dependency breakdowns. Supporting infrastructure includes type schemas for validation, internationalisation strings, HTTP headers for cross-origin isolation, and updated package configuration. Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 13
🧹 Nitpick comments (11)
app/bundler/lib/module-type.ts (1)
74-91: Type castas unknown as Expressionis fragile.The double cast on line 77 bypasses type safety. When
node.computedis true,node.propertyis anExpression, but the cast assumes this without proper type narrowing.♻️ Suggested improvement
function getStaticPropertyName(node: StaticMemberExpression): string | null { if (node.computed) { // computed property like exports["foo"] - const prop = node.property as unknown as Expression - if (isStringLiteral(prop)) { - return prop.value + // For computed properties, property is an Expression + if (isStringLiteral(node.property)) { + return (node.property as { value: string }).value } return null }app/bundler/emitter.ts (1)
72-77: Array index access could returnundefined.When
listener.length === 2, accessinglistener[index ^ 1]is safe sinceindexis 0 or 1, but TypeScript cannot verify this. The coding guidelines require checking array access by index for type safety.♻️ Suggested improvement for explicit safety
if (listener.length === 2) { // ^ flips the bit, it's either 0 or 1 here. - listener = listener[index ^ 1] + const other = listener[index ^ 1] + if (other) listener = other } else {As per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
app/bundler/lib/fetch.ts (2)
192-196: Mutating input parameternode.unpackedSize.The function modifies
node.unpackedSizeon the inputHoistedNode. While this may be intentional to backfill missing data, mutating input parameters can lead to unexpected side effects. Consider documenting this behaviour or returning the updated value separately.This could be documented in the function's JSDoc or the
HoistedNodetype could be made explicitly mutable for this use case.
178-200: Worker loop structure is correct but could be clearer.The semaphore pattern with
index++andwhile (true)works correctly, but a clearer structure might improve readability.♻️ Alternative structure (optional)
async function worker(): Promise<void> { - while (true) { - const i = index++ - if (i >= queue.length) { - break - } + let i: number + while ((i = index++) < queue.length) { const item = queue[i] if (!item) continueapp/bundler/lib/types.ts (1)
169-177: Map key inResolutionResult.packagesmay need registry disambiguation.The
packagesMap uses a string key (presumablyname@version), but if multiple registries are supported (npm and jsr as defined inRegistry), the samename@versioncould theoretically exist on both registries. Consider including the registry in the key or documenting the assumption.This may not be an issue if the resolution always operates within a single registry context, but it's worth verifying the intended usage pattern.
app/bundler/lib/registry.ts (1)
6-7: Consider reusing the existingNPM_REGISTRYconstant.The
NPM_REGISTRYconstant is already defined inshared/utils/constants.ts. Consider importing it to avoid duplication and ensure consistency.♻️ Proposed fix
+import { NPM_REGISTRY } from '#shared/utils/constants' + -const NPM_REGISTRY = 'https://registry.npmjs.org' const JSR_REGISTRY = 'https://npm.jsr.io'app/pages/impact/[...path].vue (1)
60-65: Potential null access on regex match.The regex
matchcould benullif the pattern doesn't match. While thestartsWith('@')check makes a match likely, the current code safely handles this withmatch ? match[1] : null. However,match[1]could still beundefinedif the capture group doesn't match. Consider adding a fallback.♻️ Proposed fix for robustness
const orgName = computed(() => { const name = packageName.value if (!name.startsWith('@')) return null const match = name.match(/^@([^/]+)\//) - return match ? match[1] : null + return match?.[1] ?? null })app/bundler/lib/hoist.ts (1)
136-141: Minor: consider defensive check for node existence.While
root.get(rootPkg.name)should always return a value (since it's set in the first pass), adding a defensive check would make the code more robust against future refactoring.♻️ Proposed defensive check
// second pass: process dependencies of all root packages for (const rootPkg of roots) { const node = root.get(rootPkg.name) - if (node) { + if (node !== undefined) { processDependencies(rootPkg, node) } }The current check is functionally correct; this is purely a stylistic suggestion for explicitness.
app/bundler/lib/subpaths.ts (1)
180-289: Consider splittingdiscoverSubpathsinto smaller helpers.The function is well over 50 lines and mixes export parsing, legacy fallback, and default selection, which makes it harder to test in isolation. Extracting those steps into helpers would improve readability.
As per coding guidelines: “Keep functions focused and manageable (generally under 50 lines).”
app/bundler/lib/resolve.ts (2)
66-95: Consider splittingpickVersioninto smaller helpers.The function is well over 50 lines; extracting dist-tag handling, exact-version handling, and range resolution would improve readability and test coverage.
As per coding guidelines: “Keep functions focused and manageable (generally under 50 lines).”
Also applies to: 130-143
178-277: Consider breakingresolvePackageinto smaller helpers.The function exceeds 50 lines and mixes fetch/selection, cycle handling, and dependency collection/resolution. Splitting those stages would make it easier to reason about and test.
As per coding guidelines: “Keep functions focused and manageable (generally under 50 lines).”
| if (expr.type === 'MemberExpression') { | ||
| const memberExpr = expr as StaticMemberExpression | ||
| return containsImportMeta(memberExpr.object) | ||
| } |
There was a problem hiding this comment.
containsImportMeta may access object on computed member expressions.
When expr.type === 'MemberExpression', the cast to StaticMemberExpression assumes it's non-computed, but the function doesn't verify this. For computed member expressions, the object structure may differ.
🛡️ Proposed fix
if (expr.type === 'MemberExpression') {
- const memberExpr = expr as StaticMemberExpression
- return containsImportMeta(memberExpr.object)
+ // Both computed and non-computed MemberExpressions have an object property
+ return containsImportMeta(expr.object as Expression)
}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
app/components/ImpactDependencyBar.vue (1)
47-56: Unused parameter_indexingetColor.The
_indexparameter is never used. Consider removing it.♻️ Proposed refactor
-function getColor(packageName: string, _index: number): string { +function getColor(packageName: string): string { // Use a hash of the package name for consistent colors let hash = 0 for (let i = 0; i < packageName.length; i++) { hash = (hash << 5) - hash + packageName.charCodeAt(i) hash |= 0 } return COLORS[Math.abs(hash) % COLORS.length] ?? 'bg-gray-500' }Then update the template calls:
- :class="getColor(pkg.name, index)" + :class="getColor(pkg.name)"app/bundler/emitter.ts (2)
23-26: Documentation comment is inaccurate.The JSDoc states "uses a Set internally for O(1) add/remove operations" but the implementation uses an array with
indexOf(O(n)) andtoSpliced(O(n)). Update the comment to reflect the actual implementation.📝 Proposed fix
/** * creates a typed event emitter. - * uses a Set internally for O(1) add/remove operations. + * optimizes for the common single-listener case, falling back to an array for multiple listeners. * * `@returns` an event emitter with listen and emit methods
72-77: Consider adding type assertion for array element access.When
listener.length === 2, accessinglistener[index ^ 1]is safe at runtime, but TypeScript may infer the result asCallback<T> | undefined. An assertion clarifies intent.🛡️ Proposed fix
if (listener.length === 2) { // ^ flips the bit, it's either 0 or 1 here. - listener = listener[index ^ 1] + listener = listener[index ^ 1]! } else {app/bundler/lib/module-type.ts (2)
59-66: Cast toStaticMemberExpressionshould verifycomputedproperty first.At line 60, the code casts
nodetoStaticMemberExpressionbefore checkingmemberExpr.computedat line 61. While the subsequent check prevents incorrect property access, the cast itself is misleading since computed member expressions have a different property type. Consider restructuring to checkcomputedbefore casting, or use a type guard.♻️ Proposed refactor
// module.exports if (node.type === 'MemberExpression') { - const memberExpr = node as StaticMemberExpression - if (!memberExpr.computed) { + // Check computed first before assuming static member expression structure + if (!node.computed) { + const memberExpr = node as StaticMemberExpression const obj = memberExpr.object const prop = memberExpr.property return isIdentifier(obj, 'module') && prop.type === 'Identifier' && prop.name === 'exports' } }
140-143: Unsafe cast toStaticMemberExpressionfor potentially computed member expressions.At line 142,
leftis cast toStaticMemberExpressionwithout first verifying that it's not a computed member expression. This could lead to incorrect behaviour if the AST contains computed property access likeexports["foo"] = ....♻️ Proposed fix
// exports.foo = ... or module.exports.foo = ... if (left.type === 'MemberExpression') { - const memberExpr = left as unknown as StaticMemberExpression + const memberExpr = left as { computed: boolean; object: Expression; property: unknown } const obj = memberExpr.object // direct assignment to exports.propertyName if (isExportsObject(obj)) { - const propName = getStaticPropertyName(memberExpr) + const propName = getStaticPropertyName(memberExpr as StaticMemberExpression)app/bundler/lib/fetch.ts (1)
192-195: Mutation ofnode.unpackedSizemodifies the originalHoistedNodeobject.This mutates the input data structure, which could have unintended side effects if the
HoistedResultis used elsewhere. Consider whether this is intentional or if a separate tracking mechanism would be cleaner.app/pages/package/[[org]]/[name].vue (1)
329-339: Non-null assertion onpkg.valuemay be unsafe.At line 336,
pkg.value!.nameuses a non-null assertion, but the guard only checksresolvedVersion.value. Whilepkg.valueis likely defined whenresolvedVersionexists due to data flow, this assumption isn't explicit. Consider adding a guard forpkg.valuefor defensive coding.🛡️ Proposed fix
const impactLink = computed(() => { - if (!resolvedVersion.value) return null + if (!resolvedVersion.value || !pkg.value) return null return { name: 'impact' as const, params: { - path: [pkg.value!.name, 'v', resolvedVersion.value] satisfies [string, string, string], + path: [pkg.value.name, 'v', resolvedVersion.value] satisfies [string, string, string], }, } })app/pages/impact/[...path].vue (2)
41-49: Consider usingnavigateTowithreplace: truefor SSR compatibility.Using
router.replace()in a watch may not work correctly during SSR. The NuxtnavigateTohelper with thereplaceoption handles both SSR and client-side scenarios more reliably.♻️ Proposed fix
watch( [requestedVersion, latestVersion, packageName], ([version, latest, name]) => { if (!version && latest && name) { - router.replace(`/impact/${name}/v/${latest}`) + navigateTo(`/impact/${name}/v/${latest}`, { replace: true }) } }, { immediate: true }, )
133-134: Non-null assertion onpackageSpecmay cause runtime errors.At line 134,
packageSpec!uses a non-null assertion, but thev-elsecondition only ensuresresolvedVersionexists. IfpackageNameis empty,packageSpecwould still be null. The guard at line 55 checks both, but TypeScript cannot infer this through the template condition.🛡️ Proposed fix
<!-- Main content - client only since it uses Web Workers --> - <ClientOnly v-else> - <ImpactAnalyzer :package-spec="packageSpec!" :package-name="packageName" /> + <ClientOnly v-else-if="packageSpec"> + <ImpactAnalyzer :package-spec="packageSpec" :package-name="packageName" />app/bundler/lib/subpaths.ts (1)
142-152: Potential issue withsliceoperations on file paths.At line 144,
filePath.slice(searchDir.length + 1)assumesfilePathstarts withsearchDir/. IflistFilesRecursivereturns paths that don't match this assumption,relativePathcould be incorrect.At line 151, if
suffixis longer thanrelativePath, the slice would produce an empty string or unexpected result.🛡️ Proposed defensive check
for (const filePath of allFiles) { // check if file matches the pattern + if (!filePath.startsWith(searchDir + '/')) { + continue + } const relativePath = filePath.slice(searchDir.length + 1) if (suffix && !filePath.endsWith(suffix)) { continue } // extract the wildcard match - const match = suffix ? relativePath.slice(0, relativePath.length - suffix.length) : relativePath + const match = suffix + ? relativePath.slice(0, Math.max(0, relativePath.length - suffix.length)) + : relativePathapp/bundler/lib/resolve.ts (1)
178-277: Consider splittingresolvePackageinto smaller helpers.At ~100 lines it handles selection, cycle detection, dependency collection and recursion. Extracting dependency collection and resolution into helpers would keep each function manageable.
As per coding guidelines, keep functions focused and manageable (generally under 50 lines).
mary-ext
force-pushed
the
feat/teardown
branch
from
a76f9ea to
537c945
Compare
|
I'm not sure why production knip is failing here |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
app/bundler/lib/errors.ts (1)
44-54: Consider storing thereasonparameter as a readonly field.The
reasonparameter is used in the error message but not stored as a field. This could limit debuggability when catching the error programmatically. If the reason is useful for message construction, it may also be useful for error handling code.♻️ Optional improvement
export class InvalidSpecifierError extends BundlerError { readonly specifier: string + readonly reason: string | undefined constructor(specifier: string, reason?: string) { super( reason ? `invalid specifier: ${specifier} (${reason})` : `invalid specifier: ${specifier}`, ) this.name = 'InvalidSpecifierError' this.specifier = specifier + this.reason = reason } }app/bundler/lib/fetch.ts (1)
192-195: Mutatingnode.unpackedSizedirectly may have unexpected side effects.The code mutates the
unpackedSizeproperty on theHoistedNodeobject. If callers expect thehoistedstructure to remain unchanged after callingfetchPackagesToVolume, this mutation could cause subtle bugs. Consider whether this side effect is intentional and documented, or if a separate tracking mechanism would be clearer.app/bundler/lib/resolve.ts (2)
102-110: Array access returnsundefinedwhen key doesn't exist - add explicit type narrowing.The direct property access
versions[range]andversions[cleanedVersion]could returnundefinedif the version doesn't exist. While the code handles this implicitly (returningundefinedwhich becomesnulllater), adding explicit checks would make the intent clearer and satisfy stricter type checking. As per coding guidelines, ensure type-safe array/object access.🛠️ Proposed fix
// check if range is an exact version - if (versions[range]) { - return versions[range] + const exactMatch = versions[range] + if (exactMatch) { + return exactMatch } // check cleaned version (handles v1.0.0 -> 1.0.0) const cleanedVersion = semver.clean(range, { loose: true }) - if (cleanedVersion && versions[cleanedVersion]) { - return versions[cleanedVersion] + const cleanedMatch = cleanedVersion ? versions[cleanedVersion] : undefined + if (cleanedMatch) { + return cleanedMatch }
201-213: Cycle detection returns a placeholder without dependencies.The cycle detection correctly breaks infinite recursion by returning a minimal
ResolvedPackagewith an emptydependenciesmap. This is documented, but consumers should be aware that cyclic packages won't have their full dependency tree resolved. Consider adding a flag to indicate the package is a cyclic reference.💡 Optional enhancement
+ // Note: This placeholder won't have dependencies populated + // Consumers should handle cyclic references appropriately const cyclic: ResolvedPackage = { name, version: manifest.version, tarball: manifest.dist.tarball, integrity: manifest.dist.integrity, dependencies: new Map(), + // Consider adding: cyclic: true }app/bundler/lib/hoist.ts (1)
29-53: Minor duplication:tryPlaceAtRootduplicatescreateNodelogic.Lines 34-42 manually construct a
HoistedNodeobject, duplicating the logic increateNode. Consider reusingcreateNodefor consistency and maintainability.♻️ Proposed refactor
function tryPlaceAtRoot(root: Map<string, HoistedNode>, pkg: ResolvedPackage): boolean { const existing = root.get(pkg.name) if (!existing) { // no conflict, place at root - root.set(pkg.name, { - name: pkg.name, - version: pkg.version, - tarball: pkg.tarball, - integrity: pkg.integrity, - unpackedSize: pkg.unpackedSize, - dependencyCount: pkg.dependencies.size, - nested: new Map(), - }) + root.set(pkg.name, createNode(pkg)) return true }
| switch (request.type) { | ||
| case 'init': | ||
| handleInit(request.id, request.packageSpec, request.options) | ||
| break | ||
| case 'bundle': | ||
| handleBundle(request.id, request.subpath, request.selectedExports, request.options) | ||
| break | ||
| } |
There was a problem hiding this comment.
Missing await on async handlers may cause unhandled promise rejections.
The handleInit and handleBundle functions are async, but they are called without await in the message handler. If these functions throw after their first await, the rejection won't be caught by the worker's error handling.
🛠️ Proposed fix
switch (request.type) {
case 'init':
- handleInit(request.id, request.packageSpec, request.options)
+ handleInit(request.id, request.packageSpec, request.options).catch(() => {
+ // errors are already posted to main thread in handleInit
+ })
break
case 'bundle':
- handleBundle(request.id, request.subpath, request.selectedExports, request.options)
+ handleBundle(request.id, request.subpath, request.selectedExports, request.options).catch(() => {
+ // errors are already posted to main thread in handleBundle
+ })
break
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (request.type) { | |
| case 'init': | |
| handleInit(request.id, request.packageSpec, request.options) | |
| break | |
| case 'bundle': | |
| handleBundle(request.id, request.subpath, request.selectedExports, request.options) | |
| break | |
| } | |
| switch (request.type) { | |
| case 'init': | |
| handleInit(request.id, request.packageSpec, request.options).catch(() => { | |
| // errors are already posted to main thread in handleInit | |
| }) | |
| break | |
| case 'bundle': | |
| handleBundle(request.id, request.subpath, request.selectedExports, request.options).catch(() => { | |
| // errors are already posted to main thread in handleBundle | |
| }) | |
| break | |
| } |
| const fetchOptionsSchema = v.object({ | ||
| concurrency: v.optional(v.number()), | ||
| exclude: v.optional(v.array(v.instance(RegExp))), | ||
| }) |
There was a problem hiding this comment.
RegExp instances cannot be serialised via postMessage.
The exclude field uses v.instance(RegExp), but RegExp objects cannot be cloned through the structured clone algorithm used by postMessage. If this option is passed to the worker, it will throw a DataCloneError at runtime.
Consider using string patterns instead and converting them to RegExp inside the worker.
🛠️ Proposed fix
const fetchOptionsSchema = v.object({
concurrency: v.optional(v.number()),
- exclude: v.optional(v.array(v.instance(RegExp))),
+ exclude: v.optional(v.array(v.string())), // patterns converted to RegExp in worker
})Then in the worker, convert strings to RegExp:
const excludePatterns = options.fetch?.exclude?.map(pattern => new RegExp(pattern))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchOptionsSchema = v.object({ | |
| concurrency: v.optional(v.number()), | |
| exclude: v.optional(v.array(v.instance(RegExp))), | |
| }) | |
| const fetchOptionsSchema = v.object({ | |
| concurrency: v.optional(v.number()), | |
| exclude: v.optional(v.array(v.string())), // patterns converted to RegExp in worker | |
| }) |
| // Start analysis on mount | ||
| onMounted(async () => { | ||
| try { | ||
| const result = await initPackage(props.packageSpec) | ||
|
|
||
| // Set default subpath | ||
| if (result.subpaths.defaultSubpath) { | ||
| selectedSubpath.value = result.subpaths.defaultSubpath | ||
|
|
||
| // Start initial bundle | ||
| await bundle(result.subpaths.defaultSubpath, null) | ||
| } | ||
| } catch { | ||
| // init failed — state.error will be set by useBundleAnalyzer | ||
| } | ||
| }) | ||
|
|
||
| // Re-bundle when subpath changes | ||
| let suppressExportWatch = false | ||
| watch(selectedSubpath, async newSubpath => { | ||
| if (newSubpath && state.initResult) { | ||
| suppressExportWatch = true | ||
| selectedExports.value = null // Reset to all exports | ||
| await bundle(newSubpath, null) | ||
| suppressExportWatch = false | ||
| } | ||
| }) | ||
|
|
||
| // Re-bundle when selected exports change | ||
| watch( | ||
| selectedExports, | ||
| async newExports => { | ||
| if (suppressExportWatch) return | ||
| if (selectedSubpath.value && state.initResult && state.bundleResult) { | ||
| await bundle(selectedSubpath.value, newExports) | ||
| } | ||
| }, | ||
| { deep: true }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Vue 3 watch does updating a watched ref inside onMounted trigger the watcher and its side effects
💡 Result:
Yes—if the watch() is already registered (e.g., in setup() / <script setup>) and you change the watched ref inside onMounted(), that change counts as a normal reactive mutation and will trigger the watcher (and any side effects in its callback).
Key details:
watch()is “lazy” by default: it does not run on initial mount unless you set{ immediate: true }; it runs when the watched source changes. [1]- Watcher callbacks are batched. By default (
flush: 'pre'), the callback runs in the component update cycle after parent updates (if any) and before the component’s own DOM updates; you can change timing withflush: 'post'orflush: 'sync'. [2][3] onMounted()runs after the component is mounted (DOM created/inserted), so a mutation you do there happens after the watcher was created (if created insetup()), and will therefore be observed. [4]
If you don’t want the onMounted() assignment to trigger the watcher, common patterns are: set the value before creating the watcher, or create/register the watcher inside onMounted() after your initial assignment (or use the watch handle to pause()/resume()/stop() when appropriate). [3]
Sources:
[1] Vue docs – watch() is lazy by default / immediate option: https://vuejs.org/api/reactivity-core.html
[2] Vue docs – watcher callback flush timing (default + flush: 'post'|'sync'): https://vuejs.org/guide/essentials/watchers.html
[3] Vue docs – flush options and watch handle (stop/pause/resume): https://vuejs.org/api/reactivity-core.html
[4] Vue docs – onMounted() timing: https://vuejs.org/api/composition-api-lifecycle
🏁 Script executed:
find . -type f -name "ImpactAnalyzer.vue" | head -5Repository: npmx-dev/npmx.dev
Length of output: 96
🏁 Script executed:
cat -n ./app/components/ImpactAnalyzer.vue | head -80Repository: npmx-dev/npmx.dev
Length of output: 2738
Remove the explicit bundle() call in onMounted and use finally to guarantee the flag is reset.
Setting selectedSubpath in onMounted (line 29) triggers the selectedSubpath watcher (line 41), which calls bundle() again at line 45. This causes double bundling. Additionally, if bundle() throws an error, suppressExportWatch stays true on line 43, preventing export changes from triggering future bundles. Use finally to ensure the flag is always reset:
🔧 Suggested fix
onMounted(async () => {
try {
const result = await initPackage(props.packageSpec)
// Set default subpath
if (result.subpaths.defaultSubpath) {
selectedSubpath.value = result.subpaths.defaultSubpath
- // Start initial bundle
- await bundle(result.subpaths.defaultSubpath, null)
}
} catch {
// init failed — state.error will be set by useBundleAnalyzer
}
})
watch(selectedSubpath, async newSubpath => {
if (newSubpath && state.initResult) {
suppressExportWatch = true
- selectedExports.value = null // Reset to all exports
- await bundle(newSubpath, null)
- suppressExportWatch = false
+ try {
+ selectedExports.value = null // Reset to all exports
+ await bundle(newSubpath, null)
+ } finally {
+ suppressExportWatch = false
+ }
}
})| const parsedRoute = computed(() => { | ||
| const segments = route.params.path || [] | ||
| const vIndex = segments.indexOf('v') | ||
|
|
||
| if (vIndex === -1 || vIndex >= segments.length - 1) { | ||
| return { | ||
| packageName: segments.join('/'), | ||
| version: null as string | null, | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| packageName: segments.slice(0, vIndex).join('/'), | ||
| version: segments.slice(vIndex + 1).join('/'), | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Nuxt 4 catch-all route params [...path] type string or array
💡 Result:
In Nuxt 4, a catch-all page like app/pages/[...path].vue (or [...slug].vue) produces a repeatable param, so at runtime you’ll get an array of segments:
/hello/world→route.params.pathis["hello", "world"](dev.nuxt.com)
However, in TypeScript, useRoute().params comes from vue-router, whose params are typed broadly (so a param can be string | string[]), even if your particular route always yields an array. (dev.nuxt.com)
Practical pattern:
const route = useRoute()
const path = Array.isArray(route.params.path)
? route.params.path
: [route.params.path].filter(Boolean)If you know it’s always an array for that page, you can also assert:
const path = route.params.path as string[]Citations:
- 1: https://dev.nuxt.com/docs/4.x/guide/directory-structure/pages?utm_source=openai
- 2: https://dev.nuxt.com/docs/4.x/api/composables/use-route?utm_source=openai
🌐 Web query:
Nuxt RouteParamValue type definition route.params.path
💡 Result:
In Nuxt 3 (Vue Router v4), the types come from Vue Router:
type RouteParamValue = string[1]type RouteParamsGeneric = Record<string, RouteParamValue | RouteParamValue[]>→ i.e.Record<string, string | string[]>[2]useRoute().paramsisRouteParamsGeneric[3]
So route.params.path is typed as string | string[] (for a catch-all like pages/[...path].vue, it’s typically a string[] at runtime).
const route = useRoute()
const p = route.params.path // string | string[]
const fullPath = Array.isArray(p) ? p.join('/') : pSources: [1] https://router.vuejs.org/api/type-aliases/routeparamvalue, [2] https://router.vuejs.kr/api/, [3] https://router.vuejs.org/api/interfaces/routelocationgeneric
Handle the string | string[] type of route.params.path.
Vue Router types route params generically as string | string[], even though catch-all routes in Nuxt 4 always yield arrays at runtime. The current code lacks a type guard, risking incorrect semantics if the string case occurs.
🔧 Suggested fix
const parsedRoute = computed(() => {
- const segments = route.params.path || []
+ const rawPath = route.params.path
+ const segments = Array.isArray(rawPath) ? rawPath : rawPath ? [rawPath] : []
const vIndex = segments.indexOf('v')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const parsedRoute = computed(() => { | |
| const segments = route.params.path || [] | |
| const vIndex = segments.indexOf('v') | |
| if (vIndex === -1 || vIndex >= segments.length - 1) { | |
| return { | |
| packageName: segments.join('/'), | |
| version: null as string | null, | |
| } | |
| } | |
| return { | |
| packageName: segments.slice(0, vIndex).join('/'), | |
| version: segments.slice(vIndex + 1).join('/'), | |
| } | |
| }) | |
| const parsedRoute = computed(() => { | |
| const rawPath = route.params.path | |
| const segments = Array.isArray(rawPath) ? rawPath : rawPath ? [rawPath] : [] | |
| const vIndex = segments.indexOf('v') | |
| if (vIndex === -1 || vIndex >= segments.length - 1) { | |
| return { | |
| packageName: segments.join('/'), | |
| version: null as string | null, | |
| } | |
| } | |
| return { | |
| packageName: segments.slice(0, vIndex).join('/'), | |
| version: segments.slice(vIndex + 1).join('/'), | |
| } | |
| }) |
2 participants
this is still very rough, I would need help because I am very unfamiliar with Nuxt here and with how npmx project is set up
we can extend this page with additional options later (like a
platformconfig so you can test bundle sizes with various import conditions applied)