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

refactor(errors): Node.js style validation/errors #40

Merged
merged 8 commits into from
Feb 4, 2022
Merged

refactor(errors): Node.js style validation/errors #40

merged 8 commits into from
Feb 4, 2022

Conversation

bcoe
Copy link
Collaborator

@bcoe bcoe commented Jan 22, 2022

Move to Node.js style errors and validation to make it easier to
drop into Node codebase.

Fixes #6


Note: Like with primordials, this brings us closer to Node.js codebase's style.

Move to Node.js style errors and validation to make it easier to
drop into Node codebase.

Fixes #6
errors.js Outdated
Comment on lines 3 to 13
class ERR_INVALID_ARG_TYPE extends TypeError {
constructor(name, expected, actual) {
super(`${name} must be ${expected} got ${actual}`);
}
}

class ERR_NOT_IMPLEMENTED extends Error {
constructor(feature) {
super(`${feature} not implemented`);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

by using native class, this makes the package not usable in older engines, which precludes its usage by tape.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given that the eventual target of this code is the Node.js codebase, it seems like it would be limiting to use JavaScript syntax that targets ancient engines, classes have been around since Node 6 right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since, in the Node.js codebase, we won't pull in this errors.js file (as it already has its own errors.js). I don't feel super strongly about this point, I'm more concerned about us avoiding newer syntax like ?., ??, etc. in the actual parser.

Perhaps open a discussion about what our engines target should be? and we can come back and modify syntax accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

That’s all a fair point. Is there a need to have an error subclass tho, rather than just using an error instance with a code assigned?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm now setting a code, which makes the behavior of codes. SOME_ERROR more similar to Node.js -- I'm guessing this is what's implied by Node.js placing the errors on the codes key, that the errors created will be populated with code equal to their class name.

I would like to continue using a class, so that syntax used by a consumer of errors.js is the same as in the Node.js codebase:

throw new SOME_ERROR_CODE('info')

vs.,

throw SOME_ERROR_CODE('info')

errors.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
validators.js Show resolved Hide resolved
@shadowspawn
Copy link
Collaborator

I suggest also validate multiples and shorts.

But feel free to punt, I can open an issue if you prefer, one thing at a time... 😄

@bcoe
Copy link
Collaborator Author

bcoe commented Jan 23, 2022

I suggest also validate multiples and shorts.

@shadowspawn I added multiples, let's wait to add shorts until we actually use this key (I don't think I saw it reference din the implementation yet).

@bcoe bcoe requested a review from ljharb January 23, 2022 16:21
@shadowspawn shadowspawn self-assigned this Jan 25, 2022
@shadowspawn
Copy link
Collaborator

(Marked myself as assignee to remind myself to give this a try in a program. Not claiming ownership. 😄 )

index.js Show resolved Hide resolved
@shadowspawn
Copy link
Collaborator

I'm not familiar with the node internals and more looking at the index.js end of the code, and runtime behaviour. LGTM, and runtime behaviour for an author error reasonable.

parseArgs(['--foo'], { withValue: 'foo' });
% node bad
/Users/john/Documents/Sandpits/pkgjs/parseargs/validators.js:15
    throw new ERR_INVALID_ARG_TYPE(name, 'Array', value);
    ^

ERR_INVALID_ARG_TYPE [TypeError]: options.withValue must be Array got foo
    at validateArray (/Users/john/Documents/Sandpits/pkgjs/parseargs/validators.js:15:11)
    at parseArgs (/Users/john/Documents/Sandpits/pkgjs/parseargs/index.js:92:7)
    at Object.<anonymous> (/Users/john/Documents/Sandpits/pkgjs/parseargs-issues/play/bad.js:3:1)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47 {
  code: 'ERR_INVALID_ARG_TYPE'
}

test/index.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

LGTM

@shadowspawn shadowspawn removed their assignment Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Establish error message approach
4 participants