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
+210 −27
Open

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

Show file tree
Hide file tree
Changes from 2 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
106 changes: 91 additions & 15 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 @@ -16,7 +16,7 @@
* limitations under the License.
*/

/* eslint-disable no-restricted-syntax, no-invalid-this, no-underscore-dangle, max-lines, max-lines-per-function */
/* eslint-disable no-restricted-syntax, no-invalid-this, no-underscore-dangle, max-lines */

'use strict';

Expand All @@ -26,6 +26,8 @@
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 logger = require( 'debug' );
var inherit = require( '@stdlib/utils/inherit' );
var isString = require( '@stdlib/assert/is-string' ).isPrimitive;
Expand All @@ -34,7 +36,8 @@
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 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 +46,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 +82,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 @@ -130,11 +153,10 @@
*/
function REPL( options ) {
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 +167,8 @@
self = this;

opts = defaults();
config = this._resolveConfiguration();
opts = mergeOptions( opts, config );
if ( arguments.length ) {
err = validate( opts, options );
if ( err ) {
Expand Down Expand Up @@ -285,13 +309,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 318 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 +335,10 @@
// Write a welcome message:
this._wstream.write( opts.welcome );

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

Check warning on line 338 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 340 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 @@ -415,6 +429,7 @@
if ( self._settings.bracketedPaste && self._settings.autoDisableBracketedPasteOnExit ) { // eslint-disable-line max-len
self._multilineHandler.disableBracketedPaste();
}
self._saveConfiguration( config, options );
ostream.end();
ostream.unpipe();

Expand Down Expand Up @@ -499,9 +514,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 517 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 519 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 +553,67 @@
return inputPrompt( this._inputPrompt, this._count );
});

/**
* Resolves any existing REPL configuration.
*
* @private
* @name _resolveConfiguration
* @memberof REPL.prototype
* @type {Function}
* @returns {Object} configuration object
*/
setNonEnumerableReadOnly( REPL.prototype, '_resolveConfiguration', function resolveConfiguration() {
/* eslint-disable stdlib/no-dynamic-require, no-unused-vars */
var config;
var path;

path = resolveParentPath( '.stdlib_repl.json', {
'dir': cwd()
});
if ( path ) {
config = require( path );
if ( isPlainObject( config ) ) {
return config;
}
}
path = join( homeDir(), '.stdlib', 'repl.json' );
try {
config = require( path );
if ( isPlainObject( config ) ) {
return config;
}
} catch ( e ) {
return {};
}
return {};
});

/**
* Saves preferences to REPL's default configuration file.
*
* @private
* @name _saveConfiguration
* @memberof REPL.prototype
* @type {Function}
* @param {Object} config - initial configuration
* @param {Object} opts - REPL options
* @returns {void}
*/
setNonEnumerableReadOnly( REPL.prototype, '_saveConfiguration', function saveConfiguration( config, options ) {
var newConfig;
var path;

newConfig = mergeOptions( config, pick( options, PERSISTABLE_OPTIONS ) );
newConfig.settings = this._settings;
newConfig.themes = this._syntaxHighlighter.getAllThemesConfig();
path = join( homeDir(), '.stdlib' );
if ( !dirExists( path ) ) {
mkdir( path );
}
path = join( path, 'repl.json' );
writeFileSync( path, JSON.stringify( newConfig ) );
});

/**
* Returns the current line's prompt length.
*
Expand Down Expand Up @@ -740,7 +816,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 819 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 +1420,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 1423 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 1425 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 +1445,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 1448 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 +1525,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 1528 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
20 changes: 19 additions & 1 deletion lib/node_modules/@stdlib/repl/lib/validate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

var isPlainObject = require( '@stdlib/assert/is-plain-object' );
var hasOwnProp = require( '@stdlib/assert/has-own-property' );
var objectKeys = require( '@stdlib/utils/keys' );
var mergeFcn = require( '@stdlib/utils/merge' ).factory;
var isReadableStreamLike = require( '@stdlib/assert/is-node-readable-stream-like' );
var isWritableStreamLike = require( '@stdlib/assert/is-node-writable-stream-like' );
var isString = require( '@stdlib/assert/is-string' ).isPrimitive;
Expand All @@ -32,6 +34,13 @@ var format = require( '@stdlib/string/format' );
var validateSettings = require( './validate_settings.js' );


// VARIABLES //

var extendThemes = mergeFcn({
'level': 1
});


// MAIN //

/**
Expand Down Expand Up @@ -67,7 +76,10 @@ var validateSettings = require( './validate_settings.js' );
* }
*/
function validate( opts, options ) {
var themes;
var err;
var i;

if ( !isPlainObject( options ) ) {
return new TypeError( format( 'invalid argument. Options argument must be an object. Value: `%s`.', options ) );
}
Expand Down Expand Up @@ -108,10 +120,16 @@ function validate( opts, options ) {
}
}
if ( hasOwnProp( options, 'themes' ) ) {
opts.themes = options.themes;
if ( !isPlainObject( options.themes ) ) {
return new TypeError( format( 'invalid option. `%s` option must be an object. Option: `%s`.', 'themes', options.themes ) );
}
themes = objectKeys( options.themes );
for ( i = 0; i < themes.length; i++ ) {
if ( !isPlainObject( themes[ i ] ) ) {
return new TypeError( format( 'invalid option. Each theme must be an object. Option: `%s`.', options.themes ) );
}
}
extendThemes( opts.themes, options.themes );
}
if ( hasOwnProp( options, 'save' ) ) {
opts.save = options.save;
Expand Down
Loading