Skip to content

events: convert errorMonitor to normal property - #31848

Closed
Flarna wants to merge 1 commit into
nodejs:masterfrom
dynatrace-oss-contrib:events-error-monitor
Closed

events: convert errorMonitor to normal property#31848
Flarna wants to merge 1 commit into
nodejs:masterfrom
dynatrace-oss-contrib:events-error-monitor

Conversation

@Flarna

@FlarnaFlarna commented Feb 18, 2020•
edited
Loading

Copy link
Copy Markdown
Member

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-botnodejs-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
Comment threadlib/events.js Outdated
@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
Flarnaforce-pushed the events-error-monitor branch from d1b498c to e00a3fbCompareFebruary 18, 2020 15:33
@FlarnaFlarna changed the title events: convert errorMonitor to getterevents: convert errorMonitor to normal propertyFeb 21, 2020
@FlarnaFlarna mentioned this pull request Feb 21, 2020
Closed
4 tasks
@Flarna

Flarna commented Feb 25, 2020

Copy link
Copy Markdown
MemberAuthor

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

@mmarchinimmarchini 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

nodejs-github-bot commented Feb 26, 2020

Copy link
Copy Markdown
Collaborator

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

@Flarna

Flarna commented Feb 28, 2020

Copy link
Copy Markdown
MemberAuthor

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

@FlarnaFlarna mentioned this pull request Feb 28, 2020
Closed
4 tasks
@nodejs-github-bot

nodejs-github-bot commented Feb 28, 2020

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Mar 2, 2020

Copy link
Copy Markdown
Collaborator

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

@Flarna

Flarna commented Mar 2, 2020

Copy link
Copy Markdown
MemberAuthor

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

@mmarchini

mmarchini commented Mar 2, 2020

Copy link
Copy Markdown
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 <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mmarchini

mmarchini commented Mar 2, 2020

Copy link
Copy Markdown
Contributor

Landed in ed8007a

@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 <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Mar 4, 2020
Merged
@targos

targos commented Apr 20, 2020

Copy link
Copy Markdown
Member

Depends on #30932 to land on v12.x

@ljharb

ljharb commented Apr 20, 2020

Copy link
Copy Markdown
Member

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

@targos

targos commented Apr 20, 2020

Copy link
Copy Markdown
Member

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

Flarna commented Apr 20, 2020

Copy link
Copy Markdown
MemberAuthor

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

@targostargos 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 <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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 <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targostargos mentioned this pull request May 2, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharbljharbljharb left review comments
@legendecaslegendecaslegendecas left review comments
@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@lpincalpincalpinca approved these changes
@targostargostargos approved these changes
@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@mmarchinimmarchinimmarchini approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

eventsIssues 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