fix(one-component-per-file): Ignore members imported from elsewhere#3063
fix(one-component-per-file): Ignore members imported from elsewhere#3063InSyncWithFoo wants to merge 5 commits intovuejs:masterfrom
Conversation
Co-authored-by: candy-Tong <563378816@qq.com> Co-authored-by: ota-meshi <16508807+ota-meshi@users.noreply.github.com>
🦋 Changeset detectedLatest commit: d881d20 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
FloEdelmann
left a comment
There was a problem hiding this comment.
Thank you for picking this up!
lib/utils/index.js
Outdated
| return isDestructedVueComponent ? 'defineNuxtComponent' : null | ||
| } | ||
|
|
||
| if (!isGlobalOrImportedFromVue(callee, context)) { |
There was a problem hiding this comment.
Maybe it makes sense to split this function into isGlobal and isImportedFromVue? Then the Nuxt case could be moved down again and benefit from the same treatment (as currently, creating a user function called defineNuxtComponent would trigger the same false-positive, right?)
There was a problem hiding this comment.
There are several test cases that import defineNuxtComponent from #app. It seems to be related to @unjs/unimport. I don't know Nuxt, so I can't be sure whether this treatment can be applied equally well to defineNuxtComponent.
There was a problem hiding this comment.
As for splitting, I think it's easier to refactor isGlobalOrImportedFromVue(callee, context) into isGlobalOrImportedFrom(callee, 'vue' | 'nuxt', context) than isGlobal(callee, context) || isImportedFrom(calee, 'vue' | 'nuxt', context).
There was a problem hiding this comment.
Pull request overview
Updates Vue component-definition detection to avoid vue/one-component-per-file false positives when component factory names are imported/defined from non-Vue sources, and adds regression tests + a changeset.
Changes:
- Add callee-origin checking in
getVueComponentDefinitionType()(viaReferenceTracker+findVariable()fallback) to ignore non-Vue imports/definitions. - Update rule callsites to pass
contextintogetVueComponentDefinitionType(context, node)and rename theone-component-per-filemessage id. - Expand
one-component-per-filetest coverage for Vue vs non-Vue imports/definitions; add a patch changeset entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/utils/index.js |
Adds origin-aware callee resolution and changes getVueComponentDefinitionType signature to accept context. |
lib/rules/one-component-per-file.js |
Uses new getVueComponentDefinitionType(context, node) signature and renames message id. |
lib/rules/require-name-property.js |
Updates to new getVueComponentDefinitionType(context, node) signature. |
lib/rules/require-expose.js |
Updates to new getVueComponentDefinitionType(context, node) signature. |
tests/lib/rules/one-component-per-file.test.ts |
Adds valid/invalid cases covering Vue vs non-Vue imports/definitions for component/createApp/defineComponent. |
.changeset/whole-readers-film.md |
Publishes patch note for the behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Resolves #2201, supersedes/closes #2203. Based on #2203 and this review comment.
getVueComponentDefinitionType()now also checks the source of the callee. It does this in two passes: One usingReferencesTrackerand one usingfindVariable(). This is because no methods ofReferencesTrackerreturn any results when the callee is injected, like this test inrequire-explicit-slots.test.ts:Co-authored-by: candy-Tong 563378816@qq.com
Co-authored-by: ota-meshi 16508807+ota-meshi@users.noreply.github.com