X Tutup
The Wayback Machine - https://web.archive.org/web/20221205121913/https://github.com/nodejs/node/pull/31457
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

Jump to bottom

lib: the REPL should survive deletion of Array.prototype methods #31457

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

lib: the REPL should survive deletion of Array.prototype methods #31457

wants to merge 1 commit into from

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Jan 22, 2020 •
edited

Specifically, delete Array.prototype.lastIndexOf immediately crashes the REPL, as does deletion of a few other Array prototype methods.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ljharb ljharb added domain Issues and PRs related to the domain subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 22, 2020
@ljharb ljharb requested review from bmeck and BridgeAR Jan 22, 2020
@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Jan 22, 2020
ljharb added a commit to es-shims/Array.prototype.lastIndexOf that referenced this pull request Jan 22, 2020
@ljharb
[Tests] temporarily bypass prelint
3a15042 
pending nodejs/node#31457
addaleax
addaleax approved these changes Jan 22, 2020
View changes
Copy link
Member

@addaleax addaleax left a comment

Looks like this needs a rebase?

@ljharb

This comment has been minimized.

Sign in to view
bmeck
bmeck approved these changes Jan 22, 2020
View changes
@ljharb
Copy link
Member Author

ljharb commented Jan 22, 2020

Rebased, and added a bunch of other primordials I found when poking around the repl.

ljharb
ljharb commented Jan 22, 2020
View changes
lib/repl.js Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the robustness branch 3 times, most recently from 44f2d96 to 418352d Compare Jan 22, 2020
@ljharb
Copy link
Member Author

ljharb commented Jan 22, 2020

Not sure why the python tests are failing :-/

@richardlau
Copy link
Member

richardlau commented Jan 22, 2020

Not sure why the python tests are failing :-/

For the same reason Travis is failing; there are several failing repl tests.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@ljharb ljharb force-pushed the robustness branch 2 times, most recently from c1219b5 to 907de90 Compare Jan 23, 2020
@ljharb
Copy link
Member Author

ljharb commented Jan 23, 2020

alright, got tests passing :-D this is ready for any additional/final/repeat reviews, and is ready to land whenever it's allowed to!

lundibundi
lundibundi approved these changes Jan 23, 2020
View changes
lib/domain.js Outdated Show resolved Hide resolved
@Trott

This comment has been minimized.

Sign in to view
Trott
Trott approved these changes Jan 23, 2020
View changes
@Trott
Copy link
Member

Trott commented Jan 23, 2020

I took the liberty of adding a test. Hope that's OK. (It's in a separate commit so it is easy to rebase out.)

@nodejs-github-bot
Copy link
Contributor

nodejs-github-bot commented Jan 23, 2020

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

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 30, 2020
@targos
Copy link
Member

targos commented Jan 30, 2020

The performance overhead should be much less after we land V8 8.0: https://v8.dev/blog/v8-release-80#optimizing-higher-order-builtins

@ljharb
Copy link
Member Author

ljharb commented Jan 30, 2020

I don’t think it’s true that any other program would necessarily fail in the case of these methods being removed; altho many people don’t code robustly, there are many of us that do. Either way, it’s one thing for a user modifying the env to break their own app; it’s another for it to break the platform itself.

As for the performance overhead, that’s definitely an issue :-/ but my personal philosophy is that correctness always trumps performance.

Where does this go from here? What are possible next steps?

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020
@ljharb ljharb requested a review from a team as a code owner Aug 10, 2020
@ljharb
Copy link
Member Author

ljharb commented Dec 28, 2020

Most of these changes have now landed, thanks (i believe) to @aduh95.

I've rebased this, and I think it should still land. cc @BridgeAR?

@ljharb ljharb added repl Issues and PRs related to the REPL subsystem. and removed readline Issues and PRs related to the built-in readline module. labels Dec 28, 2020
targos
targos reviewed Dec 28, 2020
View changes
lib/repl.js Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed Dec 28, 2020
View changes
lib/domain.js Outdated Show resolved Hide resolved
@ljharb
lib: the REPL should survive deletion of Array.prototype methods
12756b9 
Specifically, `delete Array.prototype.lastIndexOf` immediately crashes
the REPL, as does deletion of a few other Array prototype methods.
aduh95
aduh95 approved these changes Dec 28, 2020
View changes
Copy link
Contributor

@aduh95 aduh95 left a comment

LGTM

targos
targos approved these changes Dec 28, 2020
View changes
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2020
@nodejs-github-bot
Copy link
Contributor

nodejs-github-bot commented Dec 28, 2020

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

@PoojaDurgad PoojaDurgad added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 21, 2021
aduh95
aduh95 reviewed Jan 21, 2021
View changes
lib/repl.js
const longestNameLength = ArrayPrototypeReduce(
names,
(max, name) => MathMax(max, name.length),
0
Copy link
Contributor

@aduh95 aduh95 Jan 21, 2021

Choose a reason for hiding this comment

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

If we wait for #37005 to land, we could use MathMaxApply and get better perf results.

@jasnell jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 25, 2021
@jasnell
Copy link
Member

jasnell commented Jan 25, 2021

Removed the "Author Ready" given @BridgeAR's request changes on this.

@ljharb
Copy link
Member Author

ljharb commented Jan 25, 2021

@jasnell how long do i need to wait for a response? it’s been a month and @BridgeAR hasn't reconfirmed that the X should stand.

@jasnell
Copy link
Member

jasnell commented Jan 25, 2021

I'd just give it a couple more days, then if there's no response the objection can likely be cleared

aduh95
aduh95 reviewed Jun 13, 2021
View changes
lib/repl.js
const longestNameLength = ArrayPrototypeReduce(
names,
(max, name) => MathMax(max, name.length),
0
Copy link
Contributor

@aduh95 aduh95 Jun 13, 2021

Choose a reason for hiding this comment

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

Suggested change
const longestNameLength = ArrayPrototypeReduce(
names,
(max, name) => MathMax(max, name.length),
0
const longestNameLength = MathMaxApply(
ArrayPrototypeMap(names, (name) => name.length)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95

@BridgeAR BridgeAR

@bmeck bmeck

@jasnell jasnell

@Trott Trott

@addaleax addaleax

@targos targos

@cjihrig cjihrig

@lundibundi lundibundi

@nodejs/net net

Assignees
No one assigned
Labels
domain Issues and PRs related to the domain subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
X Tutup