vm: move some common tests to the top - #18816
Conversation
Sorry, something went wrong.
|
|
||
| function validateType(prop, propName, type = 'string') { | ||
| if (prop !== undefined && | ||
| typeof prop !== type) { |
There was a problem hiding this comment.
Nit: this should fit on the line above.
Sorry, something went wrong.
| return this.runInContext(context, options); | ||
| }; | ||
|
|
||
| function validateType(prop, propName, type = 'string') { |
There was a problem hiding this comment.
The type is always a string and setting a default value is a minor overhead. So instead, please just directly use 'string' in this function.
Sorry, something went wrong.
There was a problem hiding this comment.
In that case, please change the function name to validateString.
Sorry, something went wrong.
| } | ||
| validateType(contextOptions.name, 'options.contextName'); | ||
| validateType(contextOptions.origin, 'options.contextOrigin'); | ||
| return contextOptions; |
There was a problem hiding this comment.
When you are here: would you be so kind and rewrite this to:
if (options) {
const contextOptions = {
name: ...
...
};
validateType(contextOptions.name, 'contextOptions.name');
validate...
return contextOptions;
}
return {};
Sorry, something went wrong.
| 'string', contextOptions.origin); | ||
| } | ||
| validateType(contextOptions.name, 'options.contextName'); | ||
| validateType(contextOptions.origin, 'options.contextOrigin'); |
There was a problem hiding this comment.
Please do not change the error message itself here. So please stick to contextOptions.name and contextOptions.origin.
Sorry, something went wrong.
There was a problem hiding this comment.
@BridgeAR, the error messages are as it was.
Sorry, something went wrong.
| throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.origin', | ||
| 'string', options.origin); | ||
| } else { | ||
| validateType(options.name, 'options.name'); |
There was a problem hiding this comment.
Please keep this as it was. Otherwise there is an extra check from now on.
Sorry, something went wrong.
BridgeAR
left a comment
There was a problem hiding this comment.
LGTM with my comment addressed.
Sorry, something went wrong.
There was a problem hiding this comment.
Please add a new line in between functions.
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: nodejs#18816 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#18816 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #18816 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#18816 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Noticed some repeated tests, extract it into a function .
Checklist
Affected core subsystem(s)
vm