jshttp / cookie

HTTP server cookie parsing and serialization
MIT License
1.36k stars 159 forks source link

Feature: Throw TypeErrors in `serialize`/`parse` with .code #163

Open MaoShizhong opened 7 months ago

MaoShizhong commented 7 months ago

As requested in #162 as a non-breaking change prior to a change of error messages for a future major release, standard Node error codes have been added to each of the parse and serialize functions' TypeError throws.

This should allow for a period of migrating for library users to error-handling via the .code error property, as opposed to the sole .message property, before they change (potentially breaking).

This PR

MaoShizhong commented 6 months ago

Looks like asserting throws with a POJO was breaking backwards compatibility with the action runs. This change should hopefully be backwards compatible with the versions now.

MaoShizhong commented 6 months ago

Could I get some guidance on exactly what needs fixing here for the action runs? I'm a little confused as to why some tests that passed last run now suddenly fail after only changing the indentation for one of the files (which understandably was causing some of the later Node version tests to fail) As far as I'm concerned, the original issue regarding the test object/callback has been resolved, as has the indentation and on my end, I remember Node v0.10 and v0.12 passing respectively.

wesleytodd commented 6 months ago

Could I get some guidance on exactly what needs fixing here for the action runs?

Ah, sorry yeah I should have been more clear. I don't think you need to fix this in this PR. I think we have been considering just dropping the coveralls stuff, but it is unclear what we want bigger picture. I don't want to block landing things on this kind of unrelated stuff, so what I was thinking is I would run these tests locally once to check, but I dont see anything which should break so I hope we could just skip the check and force re-run each test run since it got past that part.

MaoShizhong commented 1 week ago

Just want to clarify what the desired way forward would be. I can update this PR with the error codes e.g.

// src/index.ts:250
if (!cookieNameRegExp.test(name)) {
  const nameError = new TypeError(`argument name is invalid: ${name}`);
  nameError.code = 'ERR_INVALID_ARG_VALUE';
  throw nameError;
}

Though since we're now using TS, I'd need to handle .code not being a property on the TypeError interface. How would you like me to proceed with that?

Also perfectly happy to have this PR superceded by #208, that's not a problem by any means (might even be preferable - very limited bandwidth currently so if this can be resolved more efficiently in that PR then that'd be wonderful).

florafinessesbeauty commented 1 week ago

It looks like you're dealing with a complex pull request related to the cookie package and ensuring TypeErrors include standard Node error codes. This kind of meticulous work is essential for robust error handling and maintaining backward compatibility.

To summarize the key points:

Error Codes in TypeErrors: The pull request adds .code properties to TypeError throws in the serialize and parse functions. This allows for more granular error handling.

Testing: Adjustments to existing tests ensure that they check for both the correct error message and the new error codes.

Performance Considerations: There's an ongoing discussion about the performance implications of creating error objects when not strictly necessary, which might impact performance in deep stacks.

Conflicting Tests: Some continuous integration (CI) tests are failing or have been canceled, particularly on older https://[node.js](https://node.js/)/ Node.jsversions. This needs further investigation to determine the exact cause and ensure that the changes do not break existing functionality.