content icon indicating copy to clipboard operation
content copied to clipboard

fix(CookieChangeEvent): revise misleading descriptions

Open xyy94813 opened this issue 9 months ago • 2 comments

Description

  • Emphasizes the existence of cookie changes.
  • Emphasizes that type belongs to cookie changes.

Motivation

The previous description misinterpreted type as referring to the type attribute of the CookieChangeEvent.

Additional details

https://wicg.github.io/cookie-store/#process-changes

Related issues and pull requests

Relates to https://github.com/mdn/translated-content/pull/19845

xyy94813 avatar May 07 '24 08:05 xyy94813

Preview URLs

(comment last updated: 2024-05-14 13:30:27)

github-actions[bot] avatar May 07 '24 08:05 github-actions[bot]

After verification, I have question about "immediately evicted".

See the follow test case:

cookieStore.addEventListener("change", (event) => {
  console.log(event)
});
// Test Case 1: set  and delete immediately 
// actually, log twice: 
// firstly, log CookieChangeEvent { changed(1), deleted(0) }
// seccondly, log CookieChangeEvent { changed(0), deleted(1) }
cookieStore.set('cookie1', '123');
cookieStore.delete('cookie1');

// Test Case 2:  set  and delete immediately 2
// actually, log twice: 
// firstly, log CookieChangeEvent { changed(1), deleted(0) }
// seccondly, log CookieChangeEvent { changed(0), deleted(1) }
const cookie3 = {
  name: 'c2',
  value: 'c2222',
}
cookieStore.set(cookie3 );
cookieStore.delete(cookie3);

// Test Case 3: set a expire cookie (passed)
// actually log once:  CookieChangeEvent { changed(0), deleted(1) }
cookieStore.set({
  name: 'c2',
  value: 'c2222',
  expires: Date.now() - 100,
});

How to define "immediately evicted" ? Is Test Case1 and Test Case2 passed?

xyy94813 avatar May 10 '24 15:05 xyy94813

The original page doesn't make sense but this PR doesn't make sense either. It seems to me like we want to say something like:

The `CookieChangeEvent` interface of the {{domxref("Cookie Store API", "", "", "nocode")}}represents a change to the contents of the cookie store. An instance of a `CookieChangeEvent` is passed into the event handler for the {{domxref("CookieStore.change_event", "change")}} event of the `CookieStore` interface.

The `CookieChangeEvent` interface has two properties to represent cookie changes:

- `changed`: an array of objects representing cookies that have been newly created and not immediately evicted.
- `deleted`: an array of objects representing cookies which have been newly created and immediately evicted, or cookies which have been otherwise evicted or removed.

Note that the `change` event ignores cookies which were removed removed due to an insertion of another cookie with the same name, domain, and path.

Wouldn't it be better to describe the details in the properties section?

xyy94813 avatar May 13 '24 04:05 xyy94813

After verification, I have question about "immediately evicted".

Thanks for raising this! I've asked for clarification in https://github.com/WICG/cookie-store/issues/229.

wbamberg avatar May 15 '24 22:05 wbamberg

The original page doesn't make sense but this PR doesn't make sense either. It seems to me like we want to say something like:

The `CookieChangeEvent` interface of the {{domxref("Cookie Store API", "", "", "nocode")}}represents a change to the contents of the cookie store. An instance of a `CookieChangeEvent` is passed into the event handler for the {{domxref("CookieStore.change_event", "change")}} event of the `CookieStore` interface.

The `CookieChangeEvent` interface has two properties to represent cookie changes:

- `changed`: an array of objects representing cookies that have been newly created and not immediately evicted.
- `deleted`: an array of objects representing cookies which have been newly created and immediately evicted, or cookies which have been otherwise evicted or removed.

Note that the `change` event ignores cookies which were removed removed due to an insertion of another cookie with the same name, domain, and path.

Wouldn't it be better to describe the details in the properties section?

Yes, probably. You mean something like:

- changed
  - : An array listing all newly-created cookies which were not immediately evicted.
- deleted
  - : An array listing all newly-created cookies which were immediately evicted, and all cookies which were otherwise evicted or removed.

?

wbamberg avatar May 15 '24 22:05 wbamberg

The original page doesn't make sense but this PR doesn't make sense either. It seems to me like we want to say something like:

The `CookieChangeEvent` interface of the {{domxref("Cookie Store API", "", "", "nocode")}}represents a change to the contents of the cookie store. An instance of a `CookieChangeEvent` is passed into the event handler for the {{domxref("CookieStore.change_event", "change")}} event of the `CookieStore` interface.

The `CookieChangeEvent` interface has two properties to represent cookie changes:

- `changed`: an array of objects representing cookies that have been newly created and not immediately evicted.
- `deleted`: an array of objects representing cookies which have been newly created and immediately evicted, or cookies which have been otherwise evicted or removed.

Note that the `change` event ignores cookies which were removed removed due to an insertion of another cookie with the same name, domain, and path.

Wouldn't it be better to describe the details in the properties section?

Yes, probably. You mean something like:

- changed
  - : An array listing all newly-created cookies which were not immediately evicted.
- deleted
  - : An array listing all newly-created cookies which were immediately evicted, and all cookies which were otherwise evicted or removed.

?

Yes.

...

## Instance properties

_This interface also inherits properties from {{domxref("Event")}}._
- {{domxref("CookieChangeEvent.changed")}} {{ReadOnlyInline}}
  - : Returns an an array of objects representing cookies that have been newly created and not immediately evicted.
- {{domxref("CookieChangeEvent.deleted")}} {{ReadOnlyInline}}
  - : `deleted`: an array of objects representing cookies which have been newly created and immediately evicted, or cookies which have been otherwise evicted or removed.

xyy94813 avatar May 16 '24 04:05 xyy94813

Based on https://github.com/WICG/cookie-store/issues/229, I wonder if it would be better to remove the "immediately evicted" language from MDN, since that seems to be more targeted at people implementing the API, rather than people using it.

something like:

- changed
  - : An array listing all newly-created cookies. Note that this will exclude cookies which were set with an expiry date in the past.
- deleted
  - : An array listing all cookies which were removed, either because they expired or because they were explicitly deleted. Note that this will include cookies which were set with an expiry date in the past.

? We could also keep an eye on https://github.com/WICG/cookie-store/issues/229 and revise this again if it seems to be needed...

Note that we do already say, in https://developer.mozilla.org/en-US/docs/Web/API/CookieStore/delete:

The delete() method expires the cookie by changing the date to one in the past.

wbamberg avatar May 16 '24 18:05 wbamberg

@xyy94813 , please let me know if you are coming back to this, if not I'm happy to try to finish it off.

wbamberg avatar May 28 '24 23:05 wbamberg

No. I haven't had enough time to focus on this issue recently.

xyy94813 avatar May 29 '24 00:05 xyy94813

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions[bot] avatar Jul 26 '24 02:07 github-actions[bot]