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.

Sign up for GitHub

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

repl: remove NODE_REPL_HISTORY_FILE #13876

Closed
wants to merge 1 commit into from
Closed

repl: remove NODE_REPL_HISTORY_FILE #13876

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

This is a alternative to #13733
The history file was only active in io.js 2.x and was deprecated in io.js 3.0. I think it's safe to remove this instead of fixing the error handling but I thought having both here to decide on is the best to do.

I'll finish the tests if it's decided that it should be removed instead of fixed.

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
Affected core subsystem(s)

repl

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem. labels Jun 22, 2017
@BridgeAR BridgeAR mentioned this pull request Jun 22, 2017
Closed
3 tasks
@cjihrig
Copy link
Contributor

cjihrig commented Jun 22, 2017

Maybe we should create an official end of life deprecation entry before removing. @jasnell WDYT?

@jasnell
Copy link
Member

jasnell commented Jun 22, 2017

Yep, I agree. Let's mark it end of life in docs/api/deprecations.md in 9.0 and look to remove in 10.0.

@BridgeAR
Copy link
Member Author

BridgeAR commented Jul 2, 2017

@cjihrig @jasnell is this really necessary in this case? The old format would immediately be migrated to the new format by starting the repl. So it's a one time process for each user that specifically used io.js 2 and it was already deprecated in io.js 3. It is a history file so would anyone want to rely on a history file that would be two years old?

@refack
Copy link
Contributor

refack commented Jul 7, 2017

@cjihrig @jasnell is this really necessary in this case? The old format would immediately be migrated to the new format by starting the repl. So it's a one time process for each user that specifically used io.js 2 and it was already deprecated in io.js 3. It is a history file so would anyone want to rely on a history file that would be two years old?

Since it's a low-risk/low-reward situation, I believe going by the book is just safer... So we'll need to mark it EOL in the docs.

@jasnell
Copy link
Member

jasnell commented Jul 9, 2017

Agree. Let's mark it EOL in 9.x

@refack refack self-assigned this Jul 9, 2017
@refack refack added this to In Progress in Error Codes Aug 20, 2017
@maclover7 maclover7 mentioned this pull request Oct 25, 2017
Closed
4 tasks
maclover7 added a commit to maclover7/node that referenced this pull request Jan 2, 2018
@maclover7
repl: runtime deprecate NODE_REPL_HISTORY_FILE
ad7c486 
Refs: nodejs#13876

Just a note that I've been trying to add tests for this, but it's been
difficult because it seems like most REPL tests directly patch into
internals, and are not run via a child process.
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 10, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 10, 2018 •
edited
Loading

I rebased this and even though the run-time deprecation did not yet land, I personally feel like it is absolutely fine to directly go to the end-of-life as this does not hurt anyone even in the very unlikely case that someone still uses io.js 2.x and directly updates to 10.x.

@BridgeAR BridgeAR requested review from jasnell, a team and cjihrig February 10, 2018 18:30
@BridgeAR
Copy link
Member Author

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

mcollina
mcollina approved these changes Feb 10, 2018
View reviewed changes
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.

LGTM

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 11, 2018
@BridgeAR
Copy link
Member Author

Landed in 60c9ad7.

@BridgeAR BridgeAR closed this Feb 12, 2018
Error Codes automation moved this from In Progress to Done Feb 12, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 12, 2018
@BridgeAR
repl: remove deprecated NODE_REPL_HISTORY_FILE
60c9ad7 
PR-URL: nodejs#13876
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@BridgeAR @MayaLekova
repl: remove deprecated NODE_REPL_HISTORY_FILE
b104daa 
PR-URL: nodejs#13876
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@refack refack removed their assignment Oct 12, 2018
@BridgeAR BridgeAR deleted the remove-old-history branch April 1, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gibfahn gibfahn gibfahn approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
No open projects
Error Codes
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@BridgeAR @cjihrig @jasnell @refack @mcollina @targos @gibfahn @nodejs-github-bot