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
New BOOTSTRAP_END event and ParameterType.Object #2040
Conversation
This is mostly a clone of `mixed` except that it accepts and processes the `oldValue` function param
@@ -503,6 +526,12 @@ const converters: { | |||
option.validate?.(value); | |||
return value; | |||
}, | |||
[ParameterType.Object](value, option, _configPath, oldValue) { | |||
option.validate?.(value); |
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 may be worth writing a default validation to check that the value type is indeed as expected or error?
There appears to be no type checking on any of the declaration type values through the abstraction layers, so the above question applies to all of them.
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.
That's a fair point, most options today silently perform a conversion... I'm not sure it matters too much since it only really affects people who write a js config file without any type checking...
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.
I was thinking more specifically of a userland scenario where a plugin is created:
app.options.addDeclaration({
name: 'whatever',
type: ParameterType.Object,
defaultValue:{...}
})
I can now call;
options.setValue('whatever', [..])
,
options.setValue('whatever', {..})
,
options.setValue('whatever', 'string')
,
options.setValue('whatever', true)
and as far as type checking and the compiler is concerned, this is all OK.
This may even be OK at runtime until some esoteric error emerges due to data corruption which will be difficult to debug.
Also, more specific to Object
only; according to type checking, defaultValue:[...]
, an array, is OK in the declaration and will safely compile. This will however cause strange behavior for the object merging, which will not fail but will break data structure and cause unexpected errors elsewhere. This is why I suggested default validators, for here something like:
//for data and maybe oldData do
if(Array.isArray(data) || typeof data !=='object`) throw new Error('data is not an Object')
The root of the problem for oldData
is that the type checking on overloads allows Mixed
(and it's clone Object
) to be an Array Object or an Object Object (curse you javascript!!!!!). I have stared at this type checking code for hours before conceding that I just cant figure how it works...
This is not really a userland problem with Mixed
, but is for Object
This is also not a critical issue. More of a pedantic twitch...
Users who declare one datatype and then set another over it probably deserve what they get. Just hope it is not me on a hangover day!
This addresses issue #2024