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.

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

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

Closed
wants to merge 1 commit into from

Conversation

chris-lsn
Copy link

related issue

#62

@what-the-diff
Copy link

what-the-diff bot commented Mar 17, 2023

  • Added a new interface to the module options
  • Created a types file for nuxt schema and added it as an extension of NuxtModule

@ennioVisco
Copy link

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

@ineshbose
Copy link
Member

ineshbose commented Mar 21, 2023

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
Copy link

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
Copy link
Member

atinux commented Mar 21, 2023

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
Copy link

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
Copy link
Author

chris-lsn commented Mar 21, 2023

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

@chris-lsn chris-lsn changed the title chore(types): fix type error if nuxtIcon property doesn't exist in appConfig fix(types): fix type error if nuxtIcon property doesn't exist in appConfig Mar 21, 2023
@ineshbose
Copy link
Member

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
Copy link
Member

ineshbose commented Mar 21, 2023

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
Copy link
Member

ineshbose commented Mar 21, 2023

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 #40)

@stafyniaksacha
Copy link

stafyniaksacha commented Mar 22, 2023

I got same issue,

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

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

@ennioVisco
Copy link

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
Copy link

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

@luke-z
Copy link

luke-z commented Mar 30, 2023

@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
Copy link

frasza commented Apr 10, 2023

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

@frasza
Copy link

frasza commented Apr 10, 2023

@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
Copy link
Member

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
Copy link

@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
Copy link

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
Copy link
Member

ineshbose commented Apr 25, 2023

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

@Pikatsuto
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants