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): improve DeepPartial type for App Config #2621

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

julien1619
Copy link

@julien1619 julien1619 commented Nov 12, 2024

πŸ”— Linked issue

Resolves #2221

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

When strict Typescript check was added, some as const where added too, and the resulting config type was showing the default value as the only possible value. I think this is not what was intended, so I treated all the as const values as string in the config type.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@julien1619 julien1619 changed the title fix(types): fix config types by removing as const (#2221) fix(types): fix config types by replacing as const (#2221) Nov 12, 2024
@julien1619
Copy link
Author

I noticed that the typecheck was failing. In fact the as const were added to send a valid value to the components which needs a strong type for some props. But the app.config.ts type is generated using only the default config.

It means that if I remove as const it improves the app.config.ts type, but breaks the typecheck of this module. So I tried adding the needed types to the default config (eg. ButtonSize, ButtonColor, etc.).

@ThibaultVlacich
Copy link

ThibaultVlacich commented Nov 13, 2024

The source of the bug are the changes made in #1870

Fixing (or rolling back) the DeepPartial type util would be a better fix imo.

@julien1619 julien1619 force-pushed the fix/typing branch 2 times, most recently from 7524b17 to d9a17fa Compare November 13, 2024 13:34
@julien1619 julien1619 changed the title fix(types): fix config types by replacing as const (#2221) fix(types): fix config types by treating const values as strings (#2221) Nov 13, 2024
@benjamincanac
Copy link
Member

It seems there is an issue in the Carousel.vue component, ui prop should be:

    ui: {
      type: Object as PropType<DeepPartial<typeof config> & { strategy?: Strategy }>,
      default: undefined
    }

@benjamincanac
Copy link
Member

benjamincanac commented Nov 13, 2024

Fixed it on dev branch, can you merge it?

@julien1619
Copy link
Author

Thanks @benjamincanac ! I've just rebased it, you can merge this PR I think :)

@benjamincanac benjamincanac changed the title fix(types): fix config types by treating const values as strings (#2221) fix(types): improve DeepPartial for App Config Nov 14, 2024
@benjamincanac benjamincanac changed the title fix(types): improve DeepPartial for App Config fix(types): improve DeepPartial type for App Config Nov 14, 2024
@benjamincanac benjamincanac merged commit 976b03f into nuxt:dev Nov 14, 2024
2 checks passed
@julien1619 julien1619 deleted the fix/typing branch December 17, 2024 16:41
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.

Custom variants not recognized by TypeScript in app.config.ts after updating to v2.18.0
3 participants