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.

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

lib: improve the usage of TypeError[INVALID_ARG_TYPE] #16401

Closed
wants to merge 1 commit into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Oct 23, 2017

The initials of expected in TypeError[ERR_INVALID_ARG_TYPE] are inconsistent.

This PR is to:

  • Unify the initial usage of TypeError[INVALID_ARG_TYPE]
  • Add an argument check before construct an error
  • Revise some inappropriate usage of INVALID_ARG_TYPE

Fixes: #16383

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

lib

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 23, 2017
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Some decisions probably need to be made re: functions and objects. Neither is technically a primitive.

Not sure how I feel about the need for checkExpectedTypeInitial — feels like reviewing PRs and modifying all existing usage would take care of its purpose.

@@ -16,7 +16,7 @@ function assertOffset(offset, length) {
}

if (offset > kMaxUint32 || offset < 0) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'offset', 'uint32');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'offset', 'Uint32');
Copy link
Member

Choose a reason for hiding this comment

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

Tempted to say that uint32 being lowercase was correct... here and in all other instances that aren't referring to Uint32Array.

@starkwang
Copy link
Contributor Author

Some decisions probably need to be made re: functions and objects. Neither is technically a primitive.

@apapirovski Agreed. But most of the codes using function and object instead of uppercase initial. I'm also confused with the strange convention.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 23, 2017
@jasnell
Copy link
Member

jasnell commented Oct 23, 2017

Defensively marking as semver-major... these type of changes should only need to be semver-major while we are still in the process of converting the errors over to the new mechanism.

@lpinca
Copy link
Member

lpinca commented Oct 24, 2017

Not sure how I feel about the need for checkExpectedTypeInitial — feels like reviewing PRs and modifying all existing usage would take care of its purpose.

👍

@joyeecheung
Copy link
Member

Defensively marking as semver-major... these type of changes should only need to be semver-major while we are still in the process of converting the errors over to the new mechanism.

@jasnell A lot of changes here only affect the error message. If this PR can split those changes touching error codes/adding additional throws out then IMO we could land the message-only changes as semver-patch.

@joyeecheung
Copy link
Member

joyeecheung commented Oct 27, 2017

Some decisions probably need to be made re: functions and objects. Neither is technically a primitive.

@apapirovski I'd say...let's go with util.isPrimitive.

> util.isPrimitive(() => {})
false
> util.isPrimitive({})
false

@apapirovski
Copy link
Member

@joyeecheung yeah, I agree — more about whether we capitalize or not. I feel like Function looks a bit weird but it's not a primitive so...

'string', 'boolean', 'number', 'object', 'undefined', 'null',
'object', 'symbol', 'false', 'falsy', 'function', 'uint32'
];
function checkExpectedTypeInitial(expectedType) {
Copy link
Member

Choose a reason for hiding this comment

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

If this remains in some form, it should be made into a lint rule. Otherwise it should IMO be removed.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Can you split the message-only changes into separate commits? (I guess we need to split the PR as well) That way they don't have to be semver-major.

Personally I prefer not to check the capitalizations of types in runtime, we can probably just make another eslint rule for it.

}

if (typeof expectedType === 'string') {
const lowercase = firstLowerCase(expectedType);
Copy link
Member

Choose a reason for hiding this comment

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

Since primitiveTypes is full of lowercase strings we can just do expectedType.toLowerCase()


if (typeof expectedType === 'string') {
const lowercase = firstLowerCase(expectedType);
if (primitiveTypes.indexOf(lowercase) === -1) {
Copy link
Member

Choose a reason for hiding this comment

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

primitiveTypes.includes(lowercase)?

@starkwang
Copy link
Contributor Author

Can you split the message-only changes into separate commits? (I guess we need to split the PR as well) That way they don't have to be semver-major.

@joyeecheung Sorry I don't understand fully. Do you mean that split every file change (about error message) into separate commits? It will be a lot of commits I guess.

Personally I prefer not to check the capitalizations of types in runtime, we can probably just make another eslint rule for it.

Agreed. I will remove it.

@joyeecheung
Copy link
Member

@starkwang Sorry for not being clear enough. Some of the changes here do not add additional throw or change the error code (i.e. do not change the first argument like "ERR_*", basically they just change the capitalization, so only the errror messages are changed), those can be split into one commit, and do not have to be sermver-major(that is, can go into 8.x). Those that do change the code or add additional throws can be split into another commit and will have to wait until 9.x

@starkwang
Copy link
Contributor Author

@joyeecheung I've just reduced this PR into message-only changes, which unifies the initials.
I will create another PR to fix the inappropriate error codes (they were in this PR before).

@joyeecheung
Copy link
Member

joyeecheung commented Oct 28, 2017

LGTM once this changes object -> Object and function -> Function, because we use the capitalized names in the documentation as well, IMO we should be more consistent on this.

Also I think I have incorrectly estimated the timeline in #16401 (comment) , from a quick glance I think none of the errors changed here can be backported in v8.x becuase they have not been migrated there in April anyway. The changes in this PR should be safe to land in v9.x and the other semver-major changes would probably need to wait for v10.x because we are very close to cut v9.x now.

@joyeecheung joyeecheung removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 28, 2017
@starkwang
Copy link
Contributor Author

Pushed commit to address comment.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with a odd finding. We should probably do an audit on the types passed to ERR_INVALID_ARG_TYPE later...

@@ -1209,7 +1209,7 @@ function toUnixTimestamp(time) {
}
throw new errors.Error('ERR_INVALID_ARG_TYPE',
'time',
['Date', 'time in seconds'],
['Date', 'Time in seconds'],
Copy link
Member

Choose a reason for hiding this comment

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

Not really need to be fixed in this PR, but this should be ['Date', 'string', 'number']

@joyeecheung
Copy link
Member

@tniessen
Copy link
Member

Personally, I usually use lowercase for things which typeof returns a correct value for, e.g. number, function and object, but I guess consistency with the documentation makes sense.

@starkwang
Copy link
Contributor Author

@jasnell @joyeecheung @tniessen Is there anyone can review or land this PR?

@joyeecheung
Copy link
Member

This is a message-only change on the migrated errors now so we don't need TSC approvals...going to land this tomorrow if no one objects. cc @nodejs/collaborators

New CI: https://ci.nodejs.org/job/node-test-pull-request/11352/

@joyeecheung joyeecheung self-assigned this Nov 11, 2017
@gireeshpunathil
Copy link
Member

for me, the types with actual casing that a compiler can successfully parse is one thing, and the types that a consumer can easily relate to is another thing, and it makes sense to abstract the strict syntax in human readable messages. However, consistency draws the bottom line.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

evanlucas pushed a commit that referenced this pull request Nov 13, 2017
The initials of expected in TypeError[ERR_INVALID_ARG_TYPE]
are inconsistent. This change is to unify them.

PR-URL: #16401
Fixes: #16383
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging or v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

As this PR touches more than 50 files it may have been nice to get a backport lined up when this landed

@joyeecheung
Copy link
Member

@MylesBorins This PR cannot land in v6.x or v8.x because it changes errors that have not been migrated in either branches, see #16401 (comment) . This should be possible to be backported to v9.x though, if it actually needs a backport.

@joyeecheung
Copy link
Member

@MylesBorins Ah nevermind, I see that it's already released on v9.2.0

starkwang added a commit to starkwang/node that referenced this pull request Dec 9, 2017
Primitives should use lowercase in error message.

refs: nodejs#16401
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 13, 2017
Primitives should use lowercase in error message.

Refs: nodejs#16401
PR-URL: nodejs#17568
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
Primitives should use lowercase in error message.

Refs: #16401
PR-URL: #17568
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 22, 2018
Primitives should use lowercase in error message.

Refs: #16401
PR-URL: #17568
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 22, 2018
Primitives should use lowercase in error message.

Refs: #16401
PR-URL: #17568
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
Primitives should use lowercase in error message.

Refs: #16401
PR-URL: #17568
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
Primitives should use lowercase in error message.

Refs: #16401
PR-URL: #17568
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
Primitives should use lowercase in error message.

Refs: #16401
PR-URL: #17568
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lib: inconsistent initial of expected in TypeError[ERR_INVALID_ARG_TYPE]