Skip to content

Conversation

@jasnell
Copy link
Member

@jasnelljasnell commented May 14, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

process (internal)

Description of change

Avoid using deprecated getter syntax plus other miscellaneous updates.

@jasnelljasnell added the process Issues and PRs related to the process subsystem. label May 14, 2016
Copy link
Member

Choose a reason for hiding this comment

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

is configurable: false, enumerable: true the default for __defineGetter__? i.e. this isn't breaking is it?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

ah, right, __defineGetter__ uses configurable: true. Will switch that.

@rvagg
Copy link
Member

+0, it's a bit churny and I'm unsure this buys us enough to warrant the churn and if we're going to churn then maybe we should make it even nicer by hoisting those functions out of there.

@jasnell
Copy link
MemberAuthor

hoisting them out and putting them where?

@jasnelljasnellforce-pushed the internal-stdio-cleanup branch from 9264bd3 to 56b5e2fCompareMay 15, 2016 14:04
@Fishrock123
Copy link
Contributor

Fishrock123 commented May 15, 2016

In the file, something seen often times as better practice.

Object.defineProperty(process,'stdout'{get: getStdout})functiongetStdout(){}

Edit: this doesn't seem to gain us anything though? +-0

@jasnell
Copy link
MemberAuthor

jasnell commented May 15, 2016

If that's the "better practice" then I'd assume it's something we'd promote across core... a quick search shows that it's not (and hasn't been) done that way consistently.

In any case, hoisted the things.

@cjihrig
Copy link
Contributor

Same comment. Churn, but the code changes themselves LGTM.

@jasnell
Copy link
MemberAuthor

@jasnell
Copy link
MemberAuthor

CI is green except for an unrelated flaky test.

Avoid using deprecated getter syntax plus other miscellaneous updates.
@jasnelljasnellforce-pushed the internal-stdio-cleanup branch from 61a26e5 to 4a828a7CompareMay 17, 2016 17:30
jasnell added a commit that referenced this pull request May 17, 2016
Avoid using deprecated getter syntax plus other miscellaneous updates. PR-URL: #6766 Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
MemberAuthor

Landed in f856234

@jasnelljasnell closed this May 17, 2016
Fishrock123 pushed a commit that referenced this pull request May 23, 2016
Avoid using deprecated getter syntax plus other miscellaneous updates. PR-URL: #6766 Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

@jasnell set to don't land.. feel free to change that

rvagg pushed a commit that referenced this pull request Jun 2, 2016
Avoid using deprecated getter syntax plus other miscellaneous updates. PR-URL: #6766 Reviewed-By: Colin Ihrig <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

processIssues and PRs related to the process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jasnell@rvagg@Fishrock123@cjihrig@MylesBorins