enjoycoding / vite-plugin-mock-server

A mock server plugin for Vite.
MIT License
58 stars 13 forks source link

Fix default values for falsy options #35

Closed WasabiThumb closed 2 months ago

WasabiThumb commented 2 months ago

As it stands it is impossible to pass a falsy value to certain config keys, namely noHandlerResponse404 and printStartupLog. This is because whether or not the default value is used depends on the truthiness of the supplied value.

options.noHandlerResponse404 = options.noHandlerResponse404 || true // (boolean | undefined) || true is always true
options.printStartupLog = options.printStartupLog || true // (boolean | undefined) || true is always true

I propose that for these keys, the truthiness of the value should be used only if it is explicitly set.

options.noHandlerResponse404 = ("noHandlerResponse404" in options) ? !!options.noHandlerResponse404 : true;
options.printStartupLog = ("printStartupLog" in options) ? !!options.printStartupLog : true;
Examples: Supplied Current Revision 1 Revision 2
true | true | true | true
{} | {} | true | true
<unspecified>* | true | true | true
false | true | false | false
0 | true | false | false
undefined | true | false | true

* Key not present in map

octoape commented 2 months ago

Hello @WasabiThumb, thank you for your PR, but I think the following fix is more appropriate,

options.noHandlerResponse404 = (typeof options.noHandlerResponse404) !== 'boolean' ? true : options.noHandlerResponse404
options.printStartupLog = (typeof options.printStartupLog) !== 'boolean' ? true : options.printStartupLog

Would you be willing to modify your PR?

WasabiThumb commented 2 months ago

Hello @WasabiThumb, thank you for your PR, but I think the following fix is more appropriate,

options.noHandlerResponse404 = (typeof options.noHandlerResponse404) !== 'boolean' ? true : options.noHandlerResponse404
options.printStartupLog = (typeof options.printStartupLog) !== 'boolean' ? true : options.printStartupLog

Would you be willing to modify your PR?

This would mean that 0 and other falsy expressions resolve to true. Some APIs return truthy/falsy values instead of literal boolean true/false. Consider a function condition that returns 0 or 1; using your proposed solution would mean that the argument must be explicitly preceded by the truthy operator !!:

{
     printStartupLog: !!condition()
}

All other arguments are used mainly when they are not undefined by use of || default (while also happening to catch NaN, null, etc). I think that changing it to check for undefined rather than key membership would be best:

options.noHandlerResponse404 = (typeof options.noHandlerResponse404 !== "undefined") ? !!options.noHandlerResponse404 : true;
options.printStartupLog = (typeof options.printStartupLog !== "undefined") ? !!options.printStartupLog : true;

Here is a precedent for this method in the popular needle request library. Consider the option parse_cookies which defaults to true. If we pass a falsy value, it is used because it is not undefined. Then this if statement respects the falsy value.

octoape commented 2 months ago

These two configurations are of boolean type, so the only valid configuration values are 'true' or 'false'. At the beginning of the design, the default value was set to 'true', which means' true 'is the default behavior and I don't want to change it. Unfortunately, your modification will change the default behavior, so I cannot merge this' PR '.

WasabiThumb commented 2 months ago

Every solution would definitionally change the behavior of the config. I'm wondering how this would be an issue though? The only case where the plugin would do something different on update is if someone already had a falsy value in the config; since there's no other issues or PRs reporting that the value does not get respected, it's safe to assume this is not the case. 🤔

At the very least, it should be documented that the options do nothing or be removed entirely and inlined to true.

WasabiThumb commented 2 months ago

I hate to beat a dead horse; but it would appear that your solution for this issue introduced an unconditional console log.