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

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MaoShizhong
Copy link
Contributor

@MaoShizhong MaoShizhong commented May 3, 2024

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

  • Amends existing tests for parse and serialize to test for the correct error message and error code.
    • The above requires a shared helper function extracted to a separate file compare-error.js
  • Adds a serialize sameSite test for non-string/true argument values.
  • Adds .code properties with standard Node error codes to each error throw in both the parse and serialize functions.

@MaoShizhong MaoShizhong changed the title Feature: Add standard codes to serialize TypeErrors Feature: Throw TypeErrors in serialize with .code May 3, 2024
@MaoShizhong MaoShizhong changed the title Feature: Throw TypeErrors in serialize with .code Feature: Throw TypeErrors in serialize/parse with .code May 3, 2024
Passing a POJO is only compatible with Node v8.0+ For backwards
compatibility, a custom callback has been passed instead.
All tests now pass even with Node v0.8 and Mocha v2.5.3
@MaoShizhong
Copy link
Contributor Author

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.

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

These CI issues are not related. In general this is great, but I think we should probably clean up the inconsistencies in formatting and also there are a few nits for after we get the CI passing.

@MaoShizhong
Copy link
Contributor Author

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
Copy link
Member

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.


if (!isDate(expires) || isNaN(expires.valueOf())) {
throw new TypeError('option expires is invalid');
var expiresError = new TypeError('option expires is invalid')
Copy link
Member

@jonchurch jonchurch Nov 15, 2024

Choose a reason for hiding this comment

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

allocating an error object when it's not needed incurs a small performance hit, due to the stack trace generation. The bigger the stack when this is generated, the bigger the hit. From microseconds to possibly miliseconds. Thats an edgecase, in the real world, but is still possible.

This is one of those instances where following Dont Repeat Yourself isn't worth the perf cost. We save a tiny amount of repetition, but at a performance cost.

@MaoShizhong
Copy link
Contributor Author

MaoShizhong commented Nov 16, 2024

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

This comment was marked as spam.

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.

4 participants