GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — process: maintain constructor descriptor by bengl · Pull Request #9306 · nodejs/node

Conversation

@bengl

@bengl bengl commented Oct 27, 2016

Copy link
Copy Markdown
Member
Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

process

Description of change

Use the original property descriptor instead of just taking the value,
which would previously, by default, be non-writable and non-configurable.

It didn't appear that it was intentional for process.constructor's descriptor
to be locked down, but it was, by using a simple {value: ...} descriptor.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bengl
process: maintain constructor descriptor
790b2f6 
Use the original property descriptor instead of just taking the value,
which would, by default, be non-writable and non-configurable.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Oct 27, 2016
@jasnell

jasnell commented Oct 27, 2016

Copy link
Copy Markdown
Member

LGTM with green CI
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bengl

bengl commented Oct 27, 2016

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/4699/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig approved these changes Oct 27, 2016
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, but it's not entirely clear to me what you're hoping to accomplish with this change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Fishrock123

Fishrock123 commented Oct 28, 2016

Copy link
Copy Markdown
Contributor

What's this aiming to solve? 🤔
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bengl

bengl commented Oct 29, 2016

Copy link
Copy Markdown
Member Author

@Fishrock123 an inconsistency in what's publicly provided in node. We're doing some automated iterating over node's public API objects, and inconsistencies can trip up our code. We can write in some exceptions, but it would be nice to not have to, especially in this case, as the non-configurability is not actually required here (it seems), and was just put there due to the way the property descriptor was originally used there.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos
targos approved these changes Nov 27, 2016
View reviewed changes
@jasnell

jasnell commented Dec 5, 2016

Copy link
Copy Markdown
Member

@Fishrock123 ... you good with this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jan 11, 2017

Copy link
Copy Markdown
Member

ping @Fishrock123
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 24, 2017

Copy link
Copy Markdown
Member

Ping... status on this one?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Mar 24, 2017
addaleax
addaleax approved these changes Mar 24, 2017
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, I see no problem with landing this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@addaleax addaleax removed the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Mar 24, 2017
@addaleax

addaleax commented Mar 24, 2017

Copy link
Copy Markdown
Member

Fresh CI: https://ci.nodejs.org/job/node-test-commit/8674/

Will land if it comes back green.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Mar 24, 2017

Copy link
Copy Markdown
Member

Landed in e0bc5a7
bengl reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax closed this Mar 24, 2017
addaleax pushed a commit that referenced this pull request Mar 24, 2017
@bengl @addaleax
process: maintain constructor descriptor
e0bc5a7 
Use the original property descriptor instead of just taking the value,
which would, by default, be non-writable and non-configurable.

PR-URL: #9306
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
@bengl @MylesBorins
process: maintain constructor descriptor
2462fd8 
Use the original property descriptor instead of just taking the value,
which would, by default, be non-writable and non-configurable.

PR-URL: #9306
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
V7.8.0 proposal #12104
Merged
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
v7.9.0 Release Proposal #12319
Merged
2 tasks done
@MylesBorins MylesBorins added the lts-watch-v6.x label Apr 18, 2017
@MylesBorins

MylesBorins commented Apr 18, 2017

Copy link
Copy Markdown
Contributor

should this be backported? it lands cleanly.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bengl

bengl commented Apr 19, 2017

Copy link
Copy Markdown
Member Author

@MylesBorins I'd say yes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels May 15, 2017
MylesBorins pushed a commit that referenced this pull request May 15, 2017
@bengl @MylesBorins
process: maintain constructor descriptor
202f007 
Use the original property descriptor instead of just taking the value,
which would, by default, be non-writable and non-configurable.

PR-URL: #9306
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
v6.11.0 proposal #13059
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@bengl @andrew749
process: maintain constructor descriptor
471a85d 
Use the original property descriptor instead of just taking the value,
which would, by default, be non-writable and non-configurable.

PR-URL: nodejs/node#9306
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
+1 more reviewer
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@bengl @jasnell @Fishrock123 @addaleax @MylesBorins @targos @cjihrig @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.