Skip to content

feat: add support for persisting user preferences using a default configuration in the REPL #2559

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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

feat: add support for persisting user preferences using a default configuration in the REPL #2559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
58eb85c
feat: add persistence using a default configuration
Snehil-Shah Jul 11, 2024
ad0cb06
fix: validate new themes
Snehil-Shah Jul 11, 2024
734560c
Merge branch 'stdlib-js:develop' into config
Snehil-Shah Jul 22, 2024
c1a4c1c
feat: allow resolving config from a `.js` export
Snehil-Shah Jul 22, 2024
1bc2c51
feat: explicitly prompt user before saving configuration
Snehil-Shah Jul 23, 2024
49b4672
refactor: replace keybinding with an input prompt
Snehil-Shah Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/node_modules/@stdlib/repl/lib/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
var stdin = require( '@stdlib/streams/node/stdin' );
var stdout = require( '@stdlib/streams/node/stdout' );
var WELCOME = require( './welcome_text.js' );
var THEMES = require( './themes.js' );


// MAIN //
Expand Down Expand Up @@ -66,6 +67,9 @@ function defaults() {
// Welcome message:
'welcome': WELCOME,

// Syntax-highlighting themes:
'themes': THEMES,

// File path specifying where to save REPL command history:
'save': '',

Expand Down
190 changes: 169 additions & 21 deletions lib/node_modules/@stdlib/repl/lib/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
var readline = require( 'readline' );
var proc = require( 'process' );
var resolve = require( 'path' ).resolve;
var join = require( 'path' ).join;
var mkdir = require( 'fs' ).mkdirSync; // eslint-disable-line node/no-sync
var dirname = require( 'path' ).dirname;
var logger = require( 'debug' );
var inherit = require( '@stdlib/utils/inherit' );
var isString = require( '@stdlib/assert/is-string' ).isPrimitive;
Expand All @@ -34,7 +37,11 @@
var isFunction = require( '@stdlib/assert/is-function' );
var isConfigurableProperty = require( '@stdlib/assert/is-configurable-property' );
var hasOwnProp = require( '@stdlib/assert/has-own-property' );
var objectKeys = require( '@stdlib/utils/keys' );
var isEmptyObject = require( '@stdlib/assert/is-empty-object' );
var deepEqual = require( '@stdlib/assert/deep-equal' );
var copy = require( '@stdlib/utils/copy' );
var pick = require( '@stdlib/utils/pick' );
var mergeFcn = require( '@stdlib/utils/merge' ).factory;
var setNonEnumerable = require( '@stdlib/utils/define-nonenumerable-property' );
var setNonEnumerableReadOnly = require( '@stdlib/utils/define-nonenumerable-read-only-property' );
var setReadOnly = require( '@stdlib/utils/define-read-only-property' );
Expand All @@ -43,7 +50,11 @@
var format = require( '@stdlib/string/format' );
var Boolean = require( '@stdlib/boolean/ctor' );
var cwd = require( '@stdlib/process/cwd' );
var resolveParentPath = require( '@stdlib/fs/resolve-parent-path' ).sync;
var homeDir = require( '@stdlib/os/homedir' );
var dirExists = require( '@stdlib/fs/exists' ).sync;
var readFileSync = require( '@stdlib/fs/read-file' ).sync;
var writeFileSync = require( '@stdlib/fs/write-file' ).sync;
var RE_EOL = require( '@stdlib/regexp/eol' ).REGEXP;
var fifo = require( '@stdlib/utils/fifo' );
var nextTick = require( '@stdlib/utils/next-tick' );
Expand Down Expand Up @@ -75,6 +86,22 @@
// VARIABLES //

var debug = logger( 'repl' );
var mergeOptions = mergeFcn({
'level': 2 // to merge individual settings and themes
});
Comment on lines +89 to +91
Copy link
Member Author

@Snehil-Shah Snehil-Shah Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am still skeptical if this is the best way to do this. For context, after resolving the options from the configuration file, I am merging it with the defaults(), till level 2. My main concern being, we might not always want to merge all things till level 2 (and things like the input and output stream properties might also get wrongly merged creating corrupted data), should we have a custom merger (similar to the validate function)?

Copy link
Member

Choose a reason for hiding this comment

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

Would you be able to show in code what you are concerned about? In particular, can you provide example config which gets merged and creates "corrupted" opts? Would help me, as I am having trouble visualizing the potential issue.

Copy link
Member Author

@Snehil-Shah Snehil-Shah Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the defaults() also has opts.input which is a stream object (with many nested properties at diff levels), say someone makes a custom configuration, with an input option to a different object, they can potentially inject and corrupt the stream object at some levels because of the merge. We do have a list of PERSISTABLE_OPTIONS (L94), that we are using to allow saving only those specific properties that we want to persist. Maybe we can use it to also enforce that only the PERSISTABLE_OPTIONS can be loaded through a custom configuration. But again, this isn't future-proof, as it can be something else other than streams as well that we might later support.

Currently, in the validate function, we manually check if the given options (at REPL instantiation) has the prop or not (for a property, say settings), if yes, we merge or replace the default with the given property manually for each option. So, should we do something similar to merge 1) defaults, 2) configuration, and 3) REPL options into a final opts object?


// List of persistable options (note: keep in alphabetical order):
var PERSISTABLE_OPTIONS = [
'inputPrompt',
'outputPrompt',
'welcome',
'padding',
'themes',
'save',
'log',
'quiet',
'settings'
];


// MAIN //
Expand Down Expand Up @@ -129,12 +156,12 @@
* repl.close();
*/
function REPL( options ) {
var configPath;
var ostream;
var themes;
var config;
var opts;
var self;
var err;
var i;

if ( !( this instanceof REPL ) ) {
if ( arguments.length ) {
Expand All @@ -145,6 +172,9 @@
self = this;

opts = defaults();
configPath = this._resolveConfigurationPath();
config = this._getConfiguration( configPath );
opts = mergeOptions( opts, config );
if ( arguments.length ) {
err = validate( opts, options );
if ( err ) {
Expand Down Expand Up @@ -184,6 +214,12 @@
// Setup the output stream pipeline:
ostream.pipe( opts.output );

// Cache references to initial REPL configuration:
setNonEnumerableReadOnly( this, '_config', config );
setNonEnumerableReadOnly( this, '_configPath', configPath );
setNonEnumerableReadOnly( this, '_options', options );
setNonEnumerableReadOnly( this, '_opts', copy( opts ) );

// Cache references to the input and output streams:
setNonEnumerableReadOnly( this, '_istream', opts.input );
setNonEnumerableReadOnly( this, '_ostream', ostream );
Expand Down Expand Up @@ -248,8 +284,8 @@
// Initialize an internal flag indicating whether the REPL is currently busy with asynchronous processing:
setNonEnumerable( this, '_busy', false );

// Initialize an internal flag indicating whether we've received a `SIGINT` signal:
setNonEnumerable( this, '_SIGINT', false );
// Initialize an internal counter indicating number of processed `SIGINT` signals:
setNonEnumerable( this, '_SIGINT', 0 );

// Initialize an internal variable for caching the result of the last successfully evaluated command:
setNonEnumerable( this, '_ans', void 0 );
Expand Down Expand Up @@ -285,13 +321,13 @@
setNonEnumerableReadOnly( this, '_previewCompleter', new PreviewCompleter( this._rli, this._completer, this._ostream, this._settings.completionPreviews ) );

// Initialize a syntax-highlighter:
setNonEnumerableReadOnly( this, '_syntaxHighlighter', new SyntaxHighlighter( this, this._ostream, this._settings.syntaxHighlighting ) );
setNonEnumerableReadOnly( this, '_syntaxHighlighter', new SyntaxHighlighter( this, this._ostream, opts.themes, this._settings.theme, this._settings.syntaxHighlighting ) );

// Cache a reference to the private readline interface `ttyWrite` to allow calling the method when wanting default behavior:
setNonEnumerableReadOnly( this, '_ttyWrite', this._rli._ttyWrite );

// Overwrite the private `ttyWrite` method to allow processing input before a "keypress" event is triggered:
this._rli._ttyWrite = beforeKeypress; // WARNING: overwriting a private property

Check warning on line 330 in lib/node_modules/@stdlib/repl/lib/main.js

View workflow job for this annotation

GitHub Actions / Lint Changed Files 

Unexpected 'warning' comment: 'WARNING: overwriting a private property'

// Add event listeners:
this._rli.on( 'close', onClose );
Expand All @@ -311,20 +347,10 @@
// Write a welcome message:
this._wstream.write( opts.welcome );

// TODO: check whether to synchronously initialize a REPL history file

Check warning on line 350 in lib/node_modules/@stdlib/repl/lib/main.js

View workflow job for this annotation

GitHub Actions / Lint Changed Files 

Unexpected 'todo' comment: 'TODO: check whether to synchronously...'

// TODO: check whether to synchronously initialize a REPL log file

Check warning on line 352 in lib/node_modules/@stdlib/repl/lib/main.js

View workflow job for this annotation

GitHub Actions / Lint Changed Files 

Unexpected 'todo' comment: 'TODO: check whether to synchronously...'

// Add any provided user-defined themes...
if ( opts.themes ) {
themes = objectKeys( opts.themes );
for ( i = 0; i < themes.length; i++ ) {
this.addTheme( themes[ i ], opts.themes[ themes[ i ] ] );
}
}
// Set the syntax highlighting theme...
this.settings( 'theme', opts.settings.theme );

// Initialize bracketed-paste:
if ( opts.settings.bracketedPaste !== void 0 ) {
this.settings( 'bracketedPaste', opts.settings.bracketedPaste );
Expand Down Expand Up @@ -400,7 +426,7 @@
* @param {string} line - line data
*/
function onLine( line ) {
self._SIGINT = false; // reset flag
self._SIGINT = 0; // reset flag
if ( self._closed === false ) {
self._multilineHandler.processLine( line );
}
Expand Down Expand Up @@ -465,15 +491,25 @@

// In the absence of any entered partial and/or unevaluated commands, determine whether we should close the REPL...
if ( self._cmd.length === 0 && isEmpty ) {
if ( self._SIGINT ) {
self._SIGINT = false;
if ( self._SIGINT === 2 ) {
self._SIGINT = 0;
self.close();
return;
}
if ( self._SIGINT === 1 ) {
if ( self._detectUnsavedChanges() ) {
self._SIGINT += 1;
self._rli.question( 'You have unsaved preferences. Do you want to save? [Y/N]: ', onAnswer );
return;
}
self._SIGINT = 0;
self.close();
return;
}
self._ostream.write( 'To exit, enter ^D or ^C again or enter quit().\n' );
self._SIGINT = true;
self._SIGINT += 1;
} else {
self._SIGINT = false;
self._SIGINT = 0;
}
// Reset the command queue:
self._queue.clear();
Expand All @@ -483,6 +519,28 @@

// Display a new prompt:
displayPrompt( self, false );

/**
* Callback invoked upon receiving an answer to a question.
*
* @private
* @param {string} answer - user answer
*/
function onAnswer( answer ) {
if ( answer === 'y' || answer === 'Y' ) {
self._saveConfiguration( config, options );
self._SIGINT = 0;
self._ostream.write( 'Preferences saved.\n' );
self.close();
return;
}
if ( answer === 'n' || answer === 'N' ) {
self._SIGINT = 0;
self.close();
return;
}
self._rli.question( 'You have unsaved preferences. Do you want to save? [Y/N]: ', onAnswer );
}
}

/**
Expand All @@ -499,9 +557,9 @@
// Update the internal command history buffer: [..., <id>, <cmd>, <success>, ...]
self._history.push( self._count, cmd, success );

// TODO: if successful and if necessary, (asynchronously?) write the command to a history file (question: do we only want to write successful commands to the history file? maybe we need to option for limiting to successful commands?)

Check warning on line 560 in lib/node_modules/@stdlib/repl/lib/main.js

View workflow job for this annotation

GitHub Actions / Lint Changed Files 

Unexpected 'todo' comment: 'TODO: if successful and if necessary,...'

// TODO: if necessary, (asynchronously?) write the command and result to a log file (JSON serialization?)

Check warning on line 562 in lib/node_modules/@stdlib/repl/lib/main.js

View workflow job for this annotation

GitHub Actions / Lint Changed Files 

Unexpected 'todo' comment: 'TODO: if necessary, (asynchronously?)...'
}
}

Expand Down Expand Up @@ -538,6 +596,96 @@
return inputPrompt( this._inputPrompt, this._count );
});

/**
* Resolves any existing REPL configuration path.
*
* @private
* @name _resolveConfigurationPath
* @memberof REPL.prototype
* @type {Function}
* @returns {string} configuration file path
*/
setNonEnumerableReadOnly( REPL.prototype, '_resolveConfigurationPath', function resolveConfigurationPath() {
var f;

f = resolveParentPath( '.stdlib_repl.json' );
if ( f ) {
return f;
}
return join( homeDir(), '.stdlib', 'repl.json' );
});

/**
* Reads from a configuration file path.
*
* @private
* @name _getConfiguration
* @memberof REPL.prototype
* @type {Function}
* @param {string} fpath - configuration file path
* @returns {Object} configuration object
*/
setNonEnumerableReadOnly( REPL.prototype, '_getConfiguration', function getConfiguration( fpath ) {
/* eslint-disable stdlib/no-dynamic-require, no-unused-vars */
var config;
try {
config = require( fpath );
if ( isPlainObject( config ) ) {
return config;
}
} catch ( e ) {
return {};
}
return {};
});

/**
* Saves preferences to a REPL configuration file.
*
* @private
* @name _saveConfiguration
* @memberof REPL.prototype
* @type {Function}
* @returns {void}
*/
setNonEnumerableReadOnly( REPL.prototype, '_saveConfiguration', function saveConfiguration() {
var newConfig;
var options;
var path;

options = pick( this._options, PERSISTABLE_OPTIONS );
newConfig = mergeOptions( this._config, options );
newConfig.settings = this._settings;
newConfig.themes = this._syntaxHighlighter.getAllThemesConfig();
path = dirname( this._configPath );
if ( !dirExists( path ) ) {
mkdir( path );
}
writeFileSync( this._configPath, JSON.stringify( newConfig, null, 2 ) );
});

/**
* Checks if there are any unsaved user preferences in the current REPL session.
*
* @private
* @name _detectUnsavedChanges
* @memberof REPL.prototype
* @type {Function}
* @returns {boolean} boolean indicating whether there are unsaved preferences in the REPL
*/
setNonEnumerableReadOnly( REPL.prototype, '_detectUnsavedChanges', function detectUnsavedChanges() {
if ( !isEmptyObject( this._options ) ) {
return true;
}
if ( !deepEqual( this._opts.settings, this._settings ) ) {
return true;
}
if ( !deepEqual( this._opts.themes, this._syntaxHighlighter.getAllThemesConfig() ) ) { // eslint-disable-line max-len
return true;
}
return false;
});

/**
* Returns the current line's prompt length.
*
Expand Down Expand Up @@ -740,7 +888,7 @@

// Before creating a new execution context in a non-sandboxed environment, remove current workspace variables in order to allow garbage collection and avoid memory leaks (e.g., variables/functions declared during a REPL session which might remain bound to the environment `global` after clearing a REPL):
if ( this._sandbox === false ) {
// WARNING: in a non-sandboxed environment, if a global variable is externally introduced during a REPL session (i.e., introduced via a mechanism outside of the REPL environment), we will delete that global variable, which means the following logic may introduce unintended side-effects for this particular edge case (e.g., application code may expect the presence of the subsequently deleted global variable). While not ideal, (a) user applications should not be introducing globals to begin with and (b) the probability of a user running a REPL session, a user clearing that REPL session, AND a global variable being introduced between starting a REPL and clearing the REPL should be negligible.

Check warning on line 891 in lib/node_modules/@stdlib/repl/lib/main.js

View workflow job for this annotation

GitHub Actions / Lint Changed Files 

Unexpected 'warning' comment: 'WARNING: in a non-sandboxed environment,...'
tmp = this._context.vars();
for ( i = 0; i < tmp.length; i++ ) {
if ( isConfigurableProperty( this._context, tmp[ i ] ) ) {
Expand Down Expand Up @@ -1344,9 +1492,9 @@
// Clear the command queue:
this._queue.clear();

// TODO: ensure REPL history is saved (flushed) to file before closing the REPL (see https://github.com/nodejs/node/blob/b21e7c7bcf23a2715951e4cd96180e4dbf1dcd4d/lib/repl.js#L805)

Check warning on line 1495 in lib/node_modules/@stdlib/repl/lib/main.js

View workflow job for this annotation

GitHub Actions / Lint Changed Files 

Unexpected 'todo' comment: 'TODO: ensure REPL history is saved...'

// TODO: ensure REPL log is saved (flushed) to file before closing the REPL

Check warning on line 1497 in lib/node_modules/@stdlib/repl/lib/main.js

View workflow job for this annotation

GitHub Actions / Lint Changed Files 

Unexpected 'todo' comment: 'TODO: ensure REPL log is saved (flushed)...'

nextTick( onTick );

Expand All @@ -1369,7 +1517,7 @@

// If this is a non-sandboxed REPL, remove global variables/properties which were introduced during context creation and by a user during a REPL session...
if ( self._sandbox === false ) {
// WARNING: in a non-sandboxed environment, if a global variable is externally introduced during a REPL session (i.e., introduced via a mechanism outside of the REPL environment), we will delete that global variable, which means the following logic may introduce unintended side-effects for this particular edge case (e.g., application code may expect the presence of the subsequently deleted global variable). While not ideal, (a) user applications should not be introducing globals to begin with and (b) the probability of a user running a REPL session, a user closing that REPL session, AND a global variable being introduced between starting a REPL and closing the REPL should be negligible.

Check warning on line 1520 in lib/node_modules/@stdlib/repl/lib/main.js

View workflow job for this annotation

GitHub Actions / Lint Changed Files 

Unexpected 'warning' comment: 'WARNING: in a non-sandboxed environment,...'
tmp = self._context.vars(); // current workspace variables
for ( i = 0; i < tmp.length; i++ ) {
if ( isConfigurableProperty( self._context, tmp[ i ] ) ) {
Expand Down Expand Up @@ -1449,7 +1597,7 @@
throw new Error( format( 'invalid argument. First argument must be a recognized setting. Value: `%s`.', name ) );
}
if ( nargs === 1 ) {
return this._settings[ name ]; // TODO: we should consider returning a deep copy if settings are allowed to be objects, not just primitives, in order to avoid unintentional mutation

Check warning on line 1600 in lib/node_modules/@stdlib/repl/lib/main.js

View workflow job for this annotation

GitHub Actions / Lint Changed Files 

Unexpected 'todo' comment: 'TODO: we should consider returning a...'
}
value = arguments[ 1 ];
f = SETTINGS_VALIDATORS[ SETTINGS[ name ].type ];
Expand Down
23 changes: 18 additions & 5 deletions lib/node_modules/@stdlib/repl/lib/syntax_highlighter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ var objectKeys = require( '@stdlib/utils/keys' );
var omit = require( '@stdlib/utils/omit' );
var hasOwnProp = require( '@stdlib/assert/has-own-property' );
var tokenizer = require( './tokenizer.js' );
var THEMES = require( './themes.js' );
var ANSI = require( './ansi_colors.js' );


Expand Down Expand Up @@ -85,12 +84,14 @@ function removeDuplicateTokens( tokens ) {
* @constructor
* @param {REPL} repl - REPL instance
* @param {WritableStream} ostream - writable stream
* @param {Object} themes - table of initial syntax-highlighting themes
* @param {theme} theme - initial color theme
* @param {boolean} enabled - boolean indicating whether the syntax-highlighter should be initially enabled
* @returns {SyntaxHighlighter} syntax-highlighter instance
*/
function SyntaxHighlighter( repl, ostream, enabled ) {
function SyntaxHighlighter( repl, ostream, themes, theme, enabled ) {
if ( !( this instanceof SyntaxHighlighter ) ) {
return new SyntaxHighlighter( repl, ostream, enabled );
return new SyntaxHighlighter( repl, ostream, themes, theme, enabled );
}
debug( 'Creating a new syntax-highlighter' );

Expand All @@ -116,10 +117,10 @@ function SyntaxHighlighter( repl, ostream, enabled ) {
this._highlightedLine = '';

// Initialize an object storing all available themes:
this._themes = THEMES;
this._themes = themes;

// Initialize a variable storing the current theme:
this._theme = DEFAULT_THEME;
this._theme = theme;

return this;
}
Expand Down Expand Up @@ -216,6 +217,18 @@ setNonEnumerableReadOnly( SyntaxHighlighter.prototype, 'getThemeConfig', functio
return this._themes[ theme ];
});

/**
* Returns all color themes.
*
* @name getAllThemesConfig
* @memberof SyntaxHighlighter.prototype
* @type {Function}
* @returns {Object} themes object
*/
setNonEnumerableReadOnly( SyntaxHighlighter.prototype, 'getAllThemesConfig', function getAllThemesConfig() {
return this._themes;
});

/**
* Adds a new color theme.
*
Expand Down
Loading
Loading