Skip to content

Conversation

@jonchurch
Copy link
Member

draft take on #163 using a custom error class, and adding in the valid type of arg when its easy to do so.

@jonchurch
Copy link
Member Author

jonchurch commented Nov 15, 2024

No bencmarks for serialize yet, so not posting bench

@jonchurch jonchurch changed the title add specific error messages to serialzie validation add specific error messages, error codes, to serialize validation Nov 15, 2024
@jonchurch
Copy link
Member Author

jonchurch commented Nov 15, 2024

@MaoShizhong don't mean to step on your contribution #163 here, started writing this before I saw your PR.

@wesleytodd I can see how error messages could be considered breaking changes for a package as widely used as cookie, aIthough I don't like it or what people would be depending on. The original change I was doing was just adding in the enriched error messages.

It does switch from TypeError to extending Error class, which is breaking since we were throwing TypeErrors before. instanceof CookieError !== 'TypeError'. I can swap it to TypeError extension, which would be nonbreaking for instanceof. But idk that feels weird? Should be named CookieSerializeValidationError or w/e at that point, instead of a generic error.

The breaking parts can be backed out, and @MaoShizhong could land the error codes independently in a minor.

@wesleytodd
Copy link
Member

I can see how error messages could be considered breaking changes for a package as widely used as cookie, aIthough I don't like it or what people would be depending on. The original change I was doing was just adding in the enriched error messages.

I would not consider this a breaking change IF we had provided .code previously. If there is a better documented way to type check an error then I am good (even if that is the less ideal typeof check which couples to implementation details). I think the issue here is that both are likely breaking in this PR.

Maybe you could break them apart and do a .code addition first?

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.

2 participants