Skip to content
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.

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

doc: add note regarding %Array.prototype.concat% in primordials.md #43166

Merged

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 21, 2022

No description provided.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 21, 2022
doc/contributing/primordials.md Outdated Show resolved Hide resolved
doc/contributing/primordials.md Outdated Show resolved Hide resolved
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 22, 2022
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

I think it would also be useful to specify that the first example is unsafe and the second one is safe, so that folks reading this doc could use what's used in the second example while replacing uses of ArrayPrototypeConcat / Array.prototype.concat. Other than that, looks good!

doc/contributing/primordials.md Show resolved Hide resolved
doc/contributing/primordials.md Show resolved Hide resolved
@LiviaMedeiros LiviaMedeiros added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 29, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 29, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/43166
✔  Done loading data for nodejs/node/pull/43166
----------------------------------- PR info ------------------------------------
Title      doc: add note regarding `%Array.prototype.concat%` in `primordials.md` (#43166)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:array-concat-prototype-tampering -> nodejs:master
Labels     doc, author ready, commit-queue-squash
Commits    4
 - doc: add note regarding `%Array.prototype.concat%` in `primordials.md`
 - fixup! doc: add note regarding `%Array.prototype.concat%` in `primord…
 - Apply suggestions from code review
 - Apply suggestions from code review
Committers 2
 - Antoine du Hamel 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/43166
Reviewed-By: LiviaMedeiros 
Reviewed-By: Darshan Sen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43166
Reviewed-By: LiviaMedeiros 
Reviewed-By: Darshan Sen 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - Apply suggestions from code review
   ℹ  This PR was created on Sat, 21 May 2022 10:50:31 GMT
   ✔  Approvals: 2
   ✔  - LiviaMedeiros (@LiviaMedeiros): https://github.com/nodejs/node/pull/43166#pullrequestreview-980911633
   ✔  - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/43166#pullrequestreview-980961722
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/2403578118

@LiviaMedeiros LiviaMedeiros added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 29, 2022
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Is it already noted that this will be significantly slower?

@LiviaMedeiros LiviaMedeiros removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 29, 2022
@nodejs-github-bot nodejs-github-bot merged commit df56644 into nodejs:master May 29, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in df56644

@LiviaMedeiros
Copy link
Contributor

@mcollina Ugh, probably not. Was there a prior discussion (or measurements) about that?

bengl pushed a commit that referenced this pull request May 30, 2022
@bengl bengl mentioned this pull request May 31, 2022
@aduh95 aduh95 deleted the array-concat-prototype-tampering branch May 31, 2022 21:13
@aduh95
Copy link
Contributor Author

aduh95 commented May 31, 2022

Is it already noted that this will be significantly slower?

I mean, SafeArrayIterator is listed as a primordials with known perf issues:

* `SafeArrayIterator`

IIRC, Array.prototype.concat is quite slow already, regardless if it's use from primordials or not.

F3n67u pushed a commit to F3n67u/node that referenced this pull request Jun 1, 2022
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
targos pushed a commit that referenced this pull request Jul 12, 2022
targos pushed a commit that referenced this pull request Jul 31, 2022
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants