Chore: Improve arg/option error messages

[MaoShizhong]

The current error messages for invalid option values (e.g. passing sameSite: 2342 or maxAge: 'cookie') are not sufficiently descriptive.

Currently, those two examples will throw option sameSite is invalid and option maxAge is invalid respectively, both of which can be interpreted as sameSite or maxAge not being valid properties on the options object.

Instead, the error messages would benefit from showing the provided value and stating that value is not a valid value for the respective option. That way, it's clear that the property is valid, just the provided value that's invalid, which is what actually is the case.

This PR

• Improves error messages to be more explicit about what caused the error
• Amends tests for error-handling to use the new error messages

[wesleytodd]

In general I am in favor of a change like this. That said, I think because we are not using .code to indicate which error folks hit, it is likely they need to parse the message to determine it which makes this a breaking change. I wonder, would you be interested in adding .code to these as a first PR we can land soon and then we can land this PR for an upcoming major version?

[MaoShizhong]

@wesleytodd Just to clarify, what exactly do you mean by "adding .code to these"?

[wesleytodd]

Ah sorry I was not clear, it is common practice to use .code to be machine readable error codes which users can use instead of the .message. For example: https://nodejs.org/docs/latest/api/errors.html#errorcode

So the idea is to first give users a better thing to do things like if (err.code === 'ERR_SAME_SITE') type checks against instead of err.message === 'the strings you just changed'. This is why changing the message here is considered "breaking".

[MaoShizhong]

@wesleytodd Apologies, I'm unfamiliar with these practices currently (for now) so I'm getting the gist but not 100% certain exactly what sort of change you would like me to do (be that in this PR or in a separate PR); hence I was under the impression that simply changing the error message would be sufficient. If you could provide an example of the change I should make that would not be breaking in this instance, I would really appreciate that.

(had I known that changing the error message itself would be considered a breaking change, I probably would've opened an issue first :sweat_smile:)

For clarity, my current understanding is that currently the errors are thrown only with messages. Therefore, current users of this library may potentially be handling errors by checking against the error's .message property, thus changing the error messages is breaking and should be saved for a major release. For now, keeping the error messages as is but simply adding an appropriate .code property to the error objects will be non-breaking, and allow people to switch to error handling via the .code property.

If that's the case, then I'd just need to know the preferred way you'd like me to implement this, whether I simply do something like

// change this
default:
  throw new TypeError('option sameSite is invalid');

// to this
default:
  var sameSiteError = new TypeError('option sameSite is invalid');
  sameSiteError.code = 'ERR_INVALID_ARG_VALUE';
  throw sameSiteError;

Or if it'd be preferable to create custom error objects with the appropriate codes and throw those as required.

[wesleytodd]

Your example is spot on! I dont think these simple errors need custom classes or anything, just what you showed would be perfect imo.

had I known that changing the error message itself would be considered a breaking change

Basically the idea is that we only have one interface for users to check which type of error was thrown, the .message property. I am being pretty pedantic about what "breaking change" means here but we have learned on this project that things are so widely used even small changes you wouldn't normally (like if you did this in app code) treat as "breaking" break users. So I just approach changes like this with an abundance of caution.

This is a GREAT direction though and I would love to see it land for the next major if we can. And to do that, landing .code in this version would be ideal to allow folks some time to migrate any checking they would do to use .code instead of .message.

[MaoShizhong]

Thank you for the confirmation and insight! TIL

I'll get a fresh PR with a .code changes up shortly, then changing the .message can be reserved for a major release which makes sense.

[wesleytodd]

To be a bit more general for future contributions, the guideline I think about is "did I have to change the existing tests for this?" If yes, then I strongly consider if it could be breaking to users in the same way.

[MaoShizhong]

That's a very good point I've not previously considered (haven't really worked on libraries much, especially something as fundamentally used as this). Would you still like me to amend the tests to add checking the .code properties in the new PR?

[wesleytodd]

I think it would be best to test both in the new PR, then we can change this one to drop the .message checks and only test the .code for the next major. I will tag this as a major and when we get to cutting that (should be soon) we can re-target it to the right release branch.

[MaoShizhong]

Excellent - appreciate the guidance on this.

[blakeembrey]

Thanks for the PR @MaoShizhong, I'm going to land this now as it looks useful for a 1.0 release. I am going to make the error message formats consistent though, so revert to the old text and append the invalid option (it's harder to read/parse if the beginning always changes).

[MaoShizhong]

@blakeembrey No problem, appreciated! Let me know if there's anything else needed from me, be that for this PR or the other one.

[blakeembrey]

Adding code is definitely useful but it can be added later as a backward compatible change, so I'm going to wait until we align on whether we want that for all the packages. Cheers!