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.

Sign up for GitHub

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

vm: move some common tests to the top #18816

Closed
wants to merge 2 commits into from
Closed

vm: move some common tests to the top #18816

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
617be72
DRY vm.js
timotew Feb 12, 2018
57b6148
validateType to validateString
timotew Feb 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 14 additions & 17 deletions lib/vm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,21 +79,21 @@ Script.prototype.runInNewContext = function(sandbox, options) {
return this.runInContext(context, options);
};

function validateType(prop, propName, type = 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, please change the function name to validateString.

if (prop !== undefined &&
typeof prop !== type) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this should fit on the line above.

throw new errors.TypeError('ERR_INVALID_ARG_TYPE', propName,
type, prop);
}
}

function getContextOptions(options) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a new line in between functions.

const contextOptions = options ? {
name: options.contextName,
origin: options.contextOrigin
} : {};
if (contextOptions.name !== undefined &&
typeof contextOptions.name !== 'string') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.contextName',
'string', contextOptions.name);
}
if (contextOptions.origin !== undefined &&
typeof contextOptions.origin !== 'string') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.contextOrigin',
'string', contextOptions.origin);
}
validateType(contextOptions.name, 'options.contextName');
validateType(contextOptions.origin, 'options.contextOrigin');
Copy link
Member

Choose a reason for hiding this comment

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

Please do not change the error message itself here. So please stick to contextOptions.name and contextOptions.origin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BridgeAR, the error messages are as it was.

return contextOptions;
Copy link
Member

Choose a reason for hiding this comment

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

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 {};

}

Expand All @@ -116,14 +116,11 @@ function createContext(sandbox, options) {
};
if (options.name === undefined) {
options.name = `VM Context ${defaultContextNameIndex++}`;
} else if (typeof options.name !== 'string') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.name',
'string', options.name);
}
if (options.origin !== undefined && typeof options.origin !== 'string') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.origin',
'string', options.origin);
} else {
validateType(options.name, 'options.name');
Copy link
Member

Choose a reason for hiding this comment

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

Please keep this as it was. Otherwise there is an extra check from now on.

}

validateType(options.origin, 'options.origin');
} else {
options = {
name: `VM Context ${defaultContextNameIndex++}`
Expand Down