Skip to content

events: convert errorMonitor to normal property #31848

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

events: convert errorMonitor to normal property #31848

wants to merge 1 commit into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Feb 18, 2020 •
edited
Loading

Convert property errorMonitor to a normal property as non-writable caused unwanted side effects.

Refs: #30932 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Feb 18, 2020
legendecas
legendecas reviewed Feb 18, 2020
View reviewed changes
@Flarna
events: convert errorMonitor to a normal property
e00a3fb 
Convert property errorMonitor to a normal property as non-writable
caused unwanted side effects.

Refs: nodejs#30932 (comment)
@Flarna Flarna force-pushed the events-error-monitor branch from d1b498c to e00a3fb Compare February 18, 2020 15:33
@Flarna Flarna changed the title events: convert errorMonitor to getter events: convert errorMonitor to normal property Feb 21, 2020
@Flarna Flarna mentioned this pull request Feb 21, 2020
Closed
4 tasks
@Flarna
Copy link
Member Author

Flarna commented Feb 25, 2020

Friendly ping, would be nice if someone could move this forward.

@mmarchini mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 26, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29358/

@Flarna
Copy link
Member Author

Flarna commented Feb 28, 2020

Is there anything I can do for CI? I haven't found a way yet to retrigger it...

@Flarna Flarna mentioned this pull request Feb 28, 2020
Closed
4 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29435/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29486/

@Flarna
Copy link
Member Author

Flarna commented Mar 2, 2020

Is there something I can do for CI? Does it help if I rebase?

@mmarchini
Copy link
Contributor

I'll land this, the only failure is

08:29:22 not ok 650 known_issues/test-vm-timeout-escape-queuemicrotask
08:29:22   ---
08:29:22   duration_ms: 1.242
08:29:22   severity: fail
08:29:22   stack: |-
08:29:22   ...

which is a known flaky (#32048) and won't be fixed (#31980).

mmarchini pushed a commit that referenced this pull request Mar 2, 2020
@Flarna @mmarchini
events: convert errorMonitor to a normal property
ed8007a 
Convert property errorMonitor to a normal property as non-writable
caused unwanted side effects.

Refs: #30932 (comment)

PR-URL: #31848
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@mmarchini
Copy link
Contributor

Landed in ed8007a

@mmarchini mmarchini closed this Mar 2, 2020
@Flarna Flarna deleted the events-error-monitor branch March 2, 2020 22:13
MylesBorins pushed a commit that referenced this pull request Mar 4, 2020
@Flarna @MylesBorins
events: convert errorMonitor to a normal property
3c8daa3 
Convert property errorMonitor to a normal property as non-writable
caused unwanted side effects.

Refs: #30932 (comment)

PR-URL: #31848
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 4, 2020
Merged
@targos
Copy link
Member

targos commented Apr 20, 2020

Depends on #30932 to land on v12.x

@ljharb
Copy link
Member

ljharb commented Apr 20, 2020

@targos to clarify; if #30932 doesn't land on 12.x, this doesn't need to either - but if it does, this must :-)

@targos
Copy link
Member

targos commented Apr 20, 2020

Yes. Sorry if I'm not giving enough details in these messages, but I have about 300 semver-patch commits to review to prepare the next v12.x release.

@Flarna
Copy link
Member Author

Flarna commented Apr 20, 2020

There is a backport PR which combines both: #32004
As #30932 is semver-minor the backport PR doesn't fit into a patch release.

@targos targos added backport-open-v12.x and removed backport-blocked-v12.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 20, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@Flarna @targos
events: convert errorMonitor to a normal property
0d60fc2 
Convert property errorMonitor to a normal property as non-writable
caused unwanted side effects.

Refs: nodejs#30932 (comment)

PR-URL: nodejs#31848
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@Flarna @targos
events: convert errorMonitor to a normal property
3241aee 
Convert property errorMonitor to a normal property as non-writable
caused unwanted side effects.

Refs: #30932 (comment)

PR-URL: #31848
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos targos mentioned this pull request May 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments

@legendecas legendecas legendecas left review comments

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@mmarchini mmarchini mmarchini approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

events Issues and PRs related to the events subsystem / EventEmitter.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@Flarna @nodejs-github-bot @mmarchini @targos @ljharb @jasnell @addaleax @lpinca @cjihrig @legendecas