Skip to content
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.

Sign up for GitHub

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: add warning if some preset import is overridden #29971

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: add warning if some preset import is overridden #29971

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0540391
feat: add warning if some preset import is overridden
OrbisK Nov 19, 2024
6aa5d12
chore: clarify duplicate import message
OrbisK Nov 19, 2024
06add6e
chore: clarify duplicate import message
OrbisK Nov 19, 2024
38bfe2a
chore: update map type
OrbisK Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions packages/nuxt/src/imports/module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,25 @@
const scannedImports = await scanDirExports(composablesDirs, {
fileFilter: file => !isIgnored(file),
})

const presetMap = new Map<string, string>()

for (const preset of presets) {
preset.imports = preset.imports ?? []
for (const i of preset.imports) {
presetMap.set(i, preset.from)
}
presetMap.set(preset.as, preset.from)
Comment on lines +124 to +128
Copy link
Contributor Author

@OrbisK OrbisK Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the previous implementation has type issues. types are weired on the presets. I tried to solve it like this, but seems kinda hacky

Suggested change
preset.imports = preset.imports ?? []
for (const i of preset.imports) {
presetMap.set(i, preset.from)
}
presetMap.set(preset.as, preset.from)
for (const i of preset.imports) {
if(typeof i === "string") {
presetMap.set(i, preset.from)
continue
}
let name, as, from
if(Array.isArray(i)) {
[name, as, from] = i
}
else {
name = ('name' in i) ? i.name : undefined
as = ('as' in i) ? i.as : undefined
from = preset.from
}
if(!name || !from) {
continue
}
presetMap.set(as ?? name, from)
}

}
danielroe marked this conversation as resolved.
Show resolved Hide resolved

for (const i of scannedImports) {

Check failure on line 131 in packages/nuxt/src/imports/module.ts

View workflow job for this annotation

GitHub Actions / build 

Argument of type 'InlinePreset | PresetImport' is not assignable to parameter of type 'string'.
const name = i.as ?? i.name
const preset = presetMap.get(name)

Check failure on line 133 in packages/nuxt/src/imports/module.ts

View workflow job for this annotation

GitHub Actions / build 

Property 'as' does not exist on type 'ImportPresetWithDeprecation'.

if (preset) {
console.warn(`[imports] "${name}" is already defined and auto imported from "${preset ?? 'unknown preset'}" within nuxt itself. Please consider renaming "${name}" at ${i.from}.`)
}
Comment on lines +132 to +137
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance the warning implementation.

While the warning implementation works, it could be improved for better developer experience and code consistency:

  1. Use the Nuxt logger utility for consistent warning format
  2. Enhance code structure with early returns
  3. Make the warning message more actionable

Apply this diff to implement these improvements:

-const name = i.as ?? i.name
-const preset = presetMap.get(name)
-
-if (preset) {
-  console.warn(`[imports] "${name}" is already defined and auto imported from "${preset ?? 'unknown preset'}" within nuxt itself. Please consider renaming "${name}" at ${i.from}.`)
-}
+const importName = i.as ?? i.name
+const presetSource = presetMap.get(importName)
+if (!presetSource) {
+  continue
+}
+
+logger.warn(
+  `Import conflict: "${importName}" is already auto-imported from preset "${presetSource}"\n` +
+  `  Source: ${i.from}\n` +
+  `  Solution: Consider renaming the import or using a different name via 'as' syntax`
+)

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Check: build

[failure] 133-133:
Property 'as' does not exist on type 'ImportPresetWithDeprecation'.


i.priority = i.priority || priorities.find(([dir]) => i.from.startsWith(dir))?.[1]
}
imports.push(...scannedImports)
Expand Down
Loading