Skip to content

lib: enforce use of Array from primordials - #30635

Closed
targos wants to merge 1 commit into
nodejs:masterfrom
targos:primordials-array
Closed

lib: enforce use of Array from primordials#30635
targos wants to merge 1 commit into
nodejs:masterfrom
targos:primordials-array

Conversation

@targos

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

@nodejs-github-botnodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 25, 2019

@targostargosNov 25, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This change is necessary, for two reasons:

  • Some of the arrays we create are returned to the user
  • Without it I'd also have to change calls to array methods

Also, It's probably better not to wrap the primordial functions for performance reasons, especially when we migrate String() and Number() calls in the future.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hmm, wait, but why is Object.setPrototypeOf(..., null) removed here?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Because if we stop wrapping the functions, it becomes useless, as there is nothing left to set the prototype of (we can't do that directly on the original functions).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. To me making the Array namespace less mutable takes precedence over keeping these functions away from prototype poisoning, so LGTM.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

joyeecheung
joyeecheung previously approved these changes Nov 27, 2019

@joyeecheungjoyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rubber Stamp LGTM

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. To me making the Array namespace less mutable takes precedence over keeping these functions away from prototype poisoning, so LGTM.

targos added a commit that referenced this pull request Nov 27, 2019
PR-URL: #30635
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos

Copy link
Copy Markdown
MemberAuthor

Landed in 141a6e3

@targostargos closed this Nov 27, 2019
@targos
targos deleted the primordials-array branch November 27, 2019 18:32
targos added a commit that referenced this pull request Dec 1, 2019
PR-URL: #30635
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BridgeARBridgeAR mentioned this pull request Dec 3, 2019
targos added a commit that referenced this pull request Jan 14, 2020
PR-URL: #30635
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targostargos mentioned this pull request Jan 15, 2020
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
PR-URL: #30635
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Feb 8, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@targos@nodejs-github-bot@jasnell@cjihrig@joyeecheung