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(type-defs): make all options optional #56

Merged
merged 2 commits into from
Dec 4, 2020
Merged

fix(type-defs): make all options optional #56

merged 2 commits into from
Dec 4, 2020

Conversation

jankal
Copy link
Contributor

@jankal jankal commented Dec 3, 2020

When using the module with a typed nuxt.config.ts, TypeScript requires all properties of the options object to be set.
They don't have to, because the defaults will be used if they are not set.

@codecov
Copy link

codecov bot commented Dec 3, 2020

Codecov Report

Merging #56 (312d781) into master (2867137) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #56   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           20        20           
  Branches         3         3           
=========================================
  Hits            20        20           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2867137...312d781. Read the comment docs.

Copy link
Contributor

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

We may also use Partial<{}>

@jankal
Copy link
Contributor Author

jankal commented Dec 3, 2020

@pi0 Thanks for that! Didn't think of it. Do you think it would be better / more consistent with other packages to adjust it? - If so, I will do. The definite pro is that you don't have to think about the question-mark on each property. A con would be the possible introduction of required properties in the future.

@pi0
Copy link
Contributor

pi0 commented Dec 4, 2020

Common convention for nuxt config is:

  • Defining an XOptions interface with all props required (considering defaults applied)
  • Defining XConfig = Partial<XOptions> for user input

Advantages:

  • When writing internals, we are sure defaults always being applied
  • No duplicate types for input and internal options
  • Ensure module remains zero config (and if a prop is really required, is checked on runtime js)

it's nuxtjs convention for ColorModeOptions to hold required properties and the config being a
Partial<T> of that
@jankal jankal requested a review from pi0 December 4, 2020 12:43
@atinux atinux merged commit 7d2aaff into nuxt-modules:master Dec 4, 2020
@atinux
Copy link
Contributor

atinux commented Dec 4, 2020

Thanks @jankal and @pi0, will release patch now :)

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.

3 participants