res.clearCookie() now ignores maxAge

[tjarbo]

This pr fixes #4851.

I have ...

• added a new test that covers my changes
• run linter

[Segmentational]

Testing the changes, in the unit-test:

  // ... blah blah blah
  [Symbol(kOutHeaders)]: [Object: null prototype] {
    'x-powered-by': [ 'X-Powered-By', 'Express' ],
    'set-cookie': [
      'Set-Cookie',
      'sid=; Path=/admin; Expires=Thu, 01 Jan 1970 00:00:00 GMT'
    ]
  }
}

confirms the maxAge attribute is indeed removed. Output following the request.end() call yields

// ... blah blah blah
 _maxListeners: undefined,
  _enableHttp2: false,
  _agent: false,
  _formData: null,
  method: 'GET',
  url: 'http://127.0.0.1:46159/',
  _header: {},
  header: {},
  writable: true,
  _redirects: 0,
  _maxRedirects: 0,
  cookies: '',
// ... blah blah blah

Great, cookies are not set in the raw http response!

Due to my little experience in our test suite, another PR would be useful. But to my understanding things are looking good.

[Segmentational]

  // ... blah blah blah
  [Symbol(kOutHeaders)]: [Object: null prototype] {
    'x-powered-by': [ 'X-Powered-By', 'Express' ],
    'set-cookie': [
      'Set-Cookie',
      'sid=; Path=/admin; Expires=Thu, 01 Jan 1970 00:00:00 GMT'
    ]
  }
}

[Segmentational]

Takes the shape:

// ... blah blah blah
 _maxListeners: undefined,
  _enableHttp2: false,
  _agent: false,
  _formData: null,
  method: 'GET',
  url: 'http://127.0.0.1:46159/',
  _header: {},
  header: {},
  writable: true,
  _redirects: 0,
  _maxRedirects: 0,
  cookies: '',
// ... blah blah blah

[Segmentational]

Looks good -- I was able to verify the expected raw http outputs

[kaj]

Excellent!

[jonchurch]

Suggested change

	var opts = merge({ expires: new Date(1), path: '/' }, options);
	delete opts.maxAge;
	var opts = merge({ path: '/' }, options);
	// Force cookie expiration by setting `expires` to a past date
	opts.expires = new Date(1);
	// Set maxAge for modern browsers to immediately delete the cookie regardless of system time
	opts.maxAge = 0

Let's do this, to also ensure they cannot overwrite expires via the options object either

[jonchurch]

Hold off on this until we get clarity on #4851 (comment) however

[jonchurch]

we can't land this in v4, so will need to target the v5 dev branch

[jonchurch]

I don't want to consider this breaking in v4, but ultimately because even an empty cookie can have semantic meaning, it is

I think it's debateable whether or not this is truly breaking in v4. I understand it is a change in implementation, but I think the implementation was bugged from the start.

You can define breaking as anything needing consumers to update their code. If folks had come to rely on the behavior here, for removing the value of a cookie and then resetting the expires into the future, then yes that would be breaking, and folks would have to update their code to use res.cookie to set a new cookie without a value.

Unfortunately, even a cookie without a value can have semantic meaning in some applications. So ughhh I guess this is breaking. I think it's a bug in v4, but it would indeed be a breaking change if someone went screwball and used this behavior on purpose in their application. Hmmmm.

I guess we can deprecate this behavior in v4 and then remove it in v5 for SURE.

[jonchurch]

We've been landing v5 changes to 5.x, which is the only branch we can really land this on currently.

So we'll need to change the target or open a new PR

We can deprecate the behavior in v4 though as well before we land this

• [v4] Deprecate res.clearCookie taking maxAge and expires parameters #5640

[jonchurch]

closing in favor of #5792