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

fix(CookieChangeEvent): revise misleading descriptions #33462

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

xyy94813
Copy link
Contributor

@xyy94813 xyy94813 commented May 7, 2024

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 mdn/translated-content#19845

@xyy94813 xyy94813 requested a review from a team as a code owner May 7, 2024 08:34
@xyy94813 xyy94813 requested review from wbamberg and removed request for a team May 7, 2024 08:34
@github-actions github-actions bot added Content:WebAPI Web API docs size/s 6-50 LoC changed labels May 7, 2024
Copy link
Contributor

github-actions bot commented May 7, 2024

Preview URLs

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

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

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.

@xyy94813
Copy link
Contributor Author

xyy94813 commented May 10, 2024

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
Copy link
Contributor Author

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 xyy94813 requested a review from wbamberg May 14, 2024 10:14
Co-authored-by: skyclouds2001 <95597335+skyclouds2001@users.noreply.github.com>
@wbamberg
Copy link
Collaborator

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

Thanks for raising this! I've asked for clarification in WICG/cookie-store#229.

@wbamberg
Copy link
Collaborator

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.

?

@xyy94813
Copy link
Contributor Author

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.

@wbamberg
Copy link
Collaborator

Based on WICG/cookie-store#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 WICG/cookie-store#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
Copy link
Collaborator

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

@xyy94813
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/s 6-50 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants