-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
fix(ts): force mandatory options #10575
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
Conversation
Closes 10573
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add tests to the plugin-test.ts
file in the types folder:
- One that checks that we can use a plugin that takes no options
- Another one that checks we can use a plugin with optional options both, with a second parameter, and without
Closes 10573
types/test/plugin-test.ts
Outdated
@@ -15,6 +15,9 @@ const plugin: PluginObject<Option> = { | |||
} | |||
const installer: PluginFunction<Option> = function(Vue, option) { } | |||
|
|||
Vue.use(plugin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one should yield an error though 🤔
The tests that must be added are:
function NoOptions(_Vue: Vue): void {}
function OptionalOption(_Vue: Vue, options?: Option): void {}
Vue.use(NoOptions)
Vue.use(OptionalOption)
Vue.use(OptionalOption, { prefix: '', suffix: '' })
I'm realizing that the types you put in vue.d.ts are different from what you tried on the TS playground
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It passes also in current dev
Thanks for that advises.
Indeed is a minor difference to the mentioned playground. The reason is
Vue.use(installer, new Option, new Option, new Option);
Would fail, since the definition hits in the master the any
case of use, which would not work with unknown
. (I am even not sure if this was intended in the master)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know, if the any
fallback was intended on Vue.use<PluginOption>
. This could behavior could be restored by a additional definition use<T>(plugin: PluginObject<T> | PluginFunction<T>, option1: any, option2: any, ...options: any[] ): VueConstructor<V>;
. It looks perhaps a bit odd, but use
and a given type should fallback at n+1( n>1 ) arguments toany
according to master.
( Please see: https://tinyurl.com/y3lcapvv )
Closes 10573
types/test/plugin-test.ts
Outdated
@@ -14,7 +14,13 @@ const plugin: PluginObject<Option> = { | |||
} | |||
} | |||
const installer: PluginFunction<Option> = function(Vue, option) { } | |||
function NoOptions( _vue: typeof Vue ){}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The typeof shouldn't be necessary, maybe we should use PluginFunction<never>
, also it's better to use an uppercase version like _Vue
or Vue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since is defined as PluginFunction<T> = (Vue: typeof _Vue, options?: T) => void;
the compiler will complain on a direct usage of Vue
as a type. I changed it to VueConstructor, so it should be clearer.
Closes 10573
@@ -111,8 +111,9 @@ export interface VueConstructor<V extends Vue = Vue> { | |||
component<Props>(id: string, definition: FunctionalComponentOptions<Props, RecordPropsDefinition<Props>>): ExtendedVue<V, {}, {}, {}, Props>; | |||
component(id: string, definition?: ComponentOptions<V>): ExtendedVue<V, {}, {}, {}, {}>; | |||
|
|||
use<T>(plugin: PluginObject<T> | PluginFunction<T>, options?: T): VueConstructor<V>; | |||
use(plugin: PluginObject<any> | PluginFunction<any>, ...options: any[]): VueConstructor<V>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test
@pkf1994 This is the 3rd time you spam with tests. Don't do it again or we will have to block you. |
Since this is now ancient...I am closing it - happy new year! |
Closes #10573
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
dev
branch for v2.x (or to a previous version branch), not themaster
branchfix #xxx[,#xxx]
, where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information:
Example: https://tinyurl.com/y2uzn6k2