nuxt / icon

The <Icon> component, supporting Iconify, Emojis and custom components.
https://stackblitz.com/edit/nuxt-icon-playground?file=app.vue
MIT License
874 stars 38 forks source link

fix(types): fix type error if nuxtIcon property doesn't exist in appConfig #63

Closed chris-lsn closed 1 year ago

chris-lsn commented 1 year ago

related issue

https://github.com/nuxt-modules/icon/issues/62

what-the-diff[bot] commented 1 year ago
ennioVisco commented 1 year ago

Well, this is not exactly a chore. This bug currently breaks builds when strict type-checking is enabled!

ineshbose commented 1 year ago

Is types.d.ts necessary? Nuxt already generates that I believe for all modules accordingly into the .nuxt directory.

I do see the intention is to extend AppConfig (but that is handled by nuxt-module-builder, see https://github.com/nuxt/module-builder/blob/844e8ddf598cf9835c250a77f6452433e3678d49/src/build.ts#L117-L123)

ennioVisco commented 1 year ago

Is types.d.ts necessary? Nuxt already generates that I believe for all modules accordingly into the .nuxt directory.

I do see the intention is to extend AppConfig

Perhaps it should be tracked as a nuxt issue. Tbh I'd say having them autogenerated is kind of an anti-pattern. What happens if two modules define the same config?

Atinux commented 1 year ago

Tbh I'd say having them autogenerated is kind of an anti-pattern.

Why is that?

What happens if two modules define the same config?

It can happen but in 5 years using Nuxt with modules, never happened

ennioVisco commented 1 year ago

Tbh I'd say having them autogenerated is kind of an anti-pattern.

Why is that?

Well, I'd expect modules to explicitly declare the interfaces, so that evolutions of the modules interface are explicitly handled by rigorous type-checking from the module authors, and not found out by users accidentally when updating.

What happens if two modules define the same config?

It can happen but in 5 years using Nuxt with modules, never happened

That said, if these cases are very infrequent/never happened, then perhaps it's reasonable to ignore them.

chris-lsn commented 1 year ago

Is types.d.ts necessary? Nuxt already generates that I believe for all modules accordingly into the .nuxt directory.

I do see the intention is to extend AppConfig (but that is handled by nuxt-module-builder, see https://github.com/nuxt/module-builder/blob/844e8ddf598cf9835c250a77f6452433e3678d49/src/build.ts#L117-L123)

I think it is necessary, nuxt auto import type only if the nuxtIcon option is defined, we could either manually typing app config or force users to add nuxtIcon option in app.config.ts to resolve this issue.

Reference: https://nuxt.com/docs/guide/directory-structure/app-config#manually-typing-app-config

ineshbose commented 1 year ago

Thanks for sharing your thoughts. Sorry for asking further, but out of curiosity I'd like to understand as well - is there any module doing this?

I missed out that NuxtConfig is different from AppConfig, but the repro in #62 doesn't use it from what I see. However, instead of creating types.d.ts with all imports from @nuxt/schema, do you think it can just be done in module.ts like:

declare module '@nuxt/schema' {
  interface AppConfig {
    nuxtIcon?: ModuleOptions
  }
}

Doesn't require any additional imports or exports! See other modules extend NuxtHooks in a similar way like so: https://github.com/nuxt-modules/tailwindcss/blob/05aaa004f5dba3cea4ceb1cd01363b1544af58dc/src/module.ts#L43-L47

ineshbose commented 1 year ago

Moreover, the module also uses the schema:extend hook (https://nuxt.com/docs/api/advanced/hooks, https://github.com/nuxt-modules/icon/blob/main/src/module.ts#L24-L64) to extend app config. If you go to https://stackblitz.com/github/nuxt-modules/icon and then playground/.nuxt/schema/nuxt.schema.d.ts, there's the generated interface. (I'm exploring this as well ๐Ÿ˜„)

ineshbose commented 1 year ago

I see the problem being raised in the runtime components in the module after changes in #56, and type configuration depends on execution for the hook so I think your solution is suitable except that I would extend the interface in module.ts than types.d.ts, and if the hook would be required in place of extending interface as it means maintaining two copies of a code imo! ๐Ÿ™‚

(Please do review the changes and description in https://github.com/nuxt-modules/icon/pull/40)

stafyniaksacha commented 1 year ago

I got same issue,

Found that adding empty object to app.config.ts solves it:

export default defineAppConfig({
  nuxtIcon: {},
})
ennioVisco commented 1 year ago

I got same issue,

Found that adding empty object to app.config.ts solves it:

export default defineAppConfig({
  nuxtIcon: {},
})

it seems to only work in dev mode :/

pratik149 commented 1 year ago

Did anyone found any solution for this issue? It is breaking the build. :(

luke-z commented 1 year ago

@pratik149 Here's my current workaround:

Put the following snippet inside รฌndex.d.ts in the root of your application.

// workaround for https://github.com/nuxt-modules/icon/pull/63
import * as _nuxt_schema from '@nuxt/schema'

interface NuxtIconModuleOptions {
  size?: string | false
  class?: string
  aliases?: { [alias: string]: string }
}

declare module '@nuxt/schema' {
  interface AppConfig {
    nuxtIcon?: NuxtIconModuleOptions
  }
}

This way it doesn't break anything for me. Gotta keep it in mind though when an official fix is released :)

frasza commented 1 year ago

@Atinux Any idea on this? Build breaking...

frasza commented 1 year ago

@luke-z with that index.d.ts snippet you provided I encounter another error:

node_modules/nuxt-icon/dist/runtime/Icon.vue:28:33 - error TS7053: Element implicitly has an 'any' type because expression of type 'string & {}' can't be used to index type '{}'.

28 const iconName = computed(() => (appConfig.nuxtIcon?.aliases || {})[props.name] || props.name)
ineshbose commented 1 year ago

Thanks for sharing your thoughts. Sorry for asking further, but out of curiosity I'd like to understand as well - is there any module doing this?

I missed out that NuxtConfig is different from AppConfig, but the repro in #62 doesn't use it from what I see. However, instead of creating types.d.ts with all imports from @nuxt/schema, do you think it can just be done in module.ts like:

declare module '@nuxt/schema' {
  interface AppConfig {
    nuxtIcon?: ModuleOptions
  }
}

Doesn't require any additional imports or exports! See other modules extend NuxtHooks in a similar way like so: https://github.com/nuxt-modules/tailwindcss/blob/05aaa004f5dba3cea4ceb1cd01363b1544af58dc/src/module.ts#L43-L47

@chris-lsn do you think this change can be made? I don't see any other issues - eager to push this for a merge ๐Ÿ™‚

pratik149 commented 1 year ago

@luke-z with that index.d.ts snippet you provided I encounter another error:

node_modules/nuxt-icon/dist/runtime/Icon.vue:28:33 - error TS7053: Element implicitly has an 'any' type because expression of type 'string & {}' can't be used to index type '{}'.

28 const iconName = computed(() => (appConfig.nuxtIcon?.aliases || {})[props.name] || props.name)

I found a temporary workaround and that is to suppress that typescript error, just so as to not break the build.

In your tsconfig.json file and in compilerOptions object, add this key-pair "suppressImplicitAnyIndexErrors": true

This is how my tsconfig.json looks like

{
  "extends": "./.nuxt/tsconfig.json",
  "compilerOptions": {
    "strict": true,
    "skipLibCheck": true,
    "suppressImplicitAnyIndexErrors": true,
    "types": [
      "@vueuse/nuxt",
      "@pinia/nuxt"
    ]
  }
}
danielgdev commented 1 year ago

Temporarily this works for me.

// app.config.ts
export interface NuxtIconModuleOptions {
  size?: string | false
  class?: string
  aliases?: { [alias: string]: string }
}

export default defineAppConfig({
  nuxtIcon: {
    size: '24px', // default <Icon> size applied
    class: 'icon', // default <Icon> class applied
    aliases: {}
  } as NuxtIconModuleOptions
})
ineshbose commented 1 year ago

Does the issue persist in recently released v0.4.0? (or keeping track of PRs mentioning #62)

Pikatsuto commented 1 year ago

Hello yes I confirm that the bug is still present because I just did the test