Skip to content

feat: add interface definitions which zone extends EventTarget#35304

Closed
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:dom-interface-zone
Closed

feat: add interface definitions which zone extends EventTarget#35304
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:dom-interface-zone

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Contributor

Close #35173

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #35173

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

add interfaces that zone.js extends EventTarget.

interface EventTarget {
  /**
   *
   * @param eventName the name of the event, such as click. This parameter is optional.
   *
   * Remove all event listeners attached to the event target, if the eventName parameter
   * is provided, will remove event listeners of specified event, if the eventName is
   * is not provided, will remove all event listeners of the event target.
   */
  removeAllListeners(eventName?: string): void;
  /**
   *
   * @param eventName the name of the event, such as click. This parameter is required.
   *
   * Get all event listeners of the specified event. return an array of event handler or
   * event listener object.
   */
  eventListeners(eventName: string): EventListenerOrEventListenerObject[];
}

@JiaLiPassion JiaLiPassion added area: zones Issues related to zone.js target: patch This PR is targeted for the next patch release labels Feb 10, 2020
@JiaLiPassion JiaLiPassion requested a review from mhevery February 10, 2020 19:28
@ngbot ngbot Bot added this to the needsTriage milestone Feb 10, 2020
@JiaLiPassion JiaLiPassion force-pushed the dom-interface-zone branch 2 times, most recently from 6f8bdea to 8d99321 Compare February 10, 2020 20:13
Copy link
Copy Markdown
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

Nits on docs, but love the result.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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


/**
  * ADD DOCS HERE. 
 */
interface EventTarget {

As of right now the license is the comment of the EventTarget which is not what you meant. Please add EventTarget docs which are separate from the license header.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You shouldn't add docs to an interface that is not owned by Angular imo. Not even sure that would even do anything at best or override the existing ones at worst.

A blank line to separate the licence should suffice, I reckon

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mhevery , got it thanks. I updated the comment here.

@alfaproject , I checked the final result, both the comments from lib.dom and zone.js will display when user mouse hover EventTarget interface.

Comment thread packages/zone.js/lib/zone.api.extensions.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Proper docs should have:

A single title sentence which goes to overview and should say just enough to tell the reader if this is what they want.

A paragraph describing the function in detail. Multiple sentences with space above it. Examples are always nice. This paragraph should be followed by `@param`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it, I updated the comment, thanks.

Comment thread packages/zone.js/lib/zone.api.extensions.ts Outdated
@mhevery mhevery added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 11, 2020
@JiaLiPassion JiaLiPassion force-pushed the dom-interface-zone branch 2 times, most recently from 4f6edd3 to 8c99b87 Compare February 12, 2020 05:00
@JiaLiPassion JiaLiPassion requested a review from mhevery February 20, 2020 22:34
@JiaLiPassion JiaLiPassion removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 20, 2020
@JiaLiPassion JiaLiPassion force-pushed the dom-interface-zone branch 2 times, most recently from a563555 to 2e8d6a2 Compare February 24, 2020 04:24
Copy link
Copy Markdown
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

with some doc nits

Comment thread packages/zone.js/lib/zone.api.extensions.ts Outdated
Comment thread packages/zone.js/lib/zone.api.extensions.ts Outdated
Comment thread packages/zone.js/lib/zone.api.extensions.ts Outdated
Comment thread packages/zone.js/lib/zone.api.extensions.ts Outdated
Comment thread packages/zone.js/lib/zone.api.extensions.ts Outdated
Comment thread packages/zone.js/lib/zone.api.extensions.ts Outdated
Comment thread packages/zone.js/lib/zone.api.extensions.ts Outdated
@mhevery mhevery added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 26, 2020
@JiaLiPassion JiaLiPassion removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 26, 2020
@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Feb 26, 2020
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Feb 26, 2020

presubmit

@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: zones Issues related to zone.js cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add interfaces zone.js added to EventTarget

4 participants