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

[Snehil-Shah]

Resolves #2510

Description

What is the purpose of this pull request?

This pull request:

• adds support for automatic loading and saving of a default configuration for persisting user preferences in a ~/.stdlib/repl.json file.

Related Issues

Does this pull request have any related issues?

This pull request:

• resolves [RFC]: add support for persisting REPL configuration across sessions #2510

Questions

Any questions for reviewers of this pull request?

• These are the options we are currently persisting by default:

'inputPrompt',
'outputPrompt',
'welcome',
'padding',
'themes',
'save',
'log',
'quiet',
'settings'

We are not persisting:

'sandbox',
'timeout',
'isTTY',
'load'

Is this okay?

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

• Read, understood, and followed the contributing guidelines.

---

@stdlib-js/reviewers

[Snehil-Shah]

Regarding hardReset, should it also reset the current REPL session's settings and preferences? To achieve this, we will have to manually change the prompt, padding and all other instantiation options..

[kgryte]

I would think that a hard reset would only reset back to the state of the REPL at instantiation. So whatever is passed or used in the constructor, that should be restored.

[Snehil-Shah]

What about configuration then? What if the user wants to reset back to the very default settings (like deleting the configuration file)

[kgryte]

What about configuration then? What if the user wants to reset back to the very default settings (like deleting the configuration file)

Hmm...I am not sure. I'd say let's punt on this. If a user wants to reset back to default settings, they can do as you say: namely, delete the configuration file. I don't think we need a command for doing this.

[Snehil-Shah]

Currently, if a user loads a custom config, that is saved to the default config upon closing. Should this be expected behavior?

[kgryte]

Currently, if a user loads a custom config, that is saved to the default config upon closing. Should this be expected behavior?

I don't think this should be expected behavior. Simply because I load a config into the REPL does not mean that I want it saved as the default. It's fine to save it as the "last session" config, but not the default.

[Snehil-Shah]

By default I meant the last session config. Just to clear things up, by last session config do you mean <homedir>/.stdlib/repl.json that we auto load each session?

[kgryte]

I'm referring to this comment: #2510 (comment).

Although, my mind may not be entirely clear on this, as we've iterated on what configs should be stored and my mental model may be outdated at this point.

[kgryte]

In short, it is not clear to me that what is loaded by default equals what state the settings were when the REPL last closed.

[kgryte]

...certain things make sense to persist and be the default next time around (e.g., themes?), but other things less so. E.g., if I temporarily disable auto-closing and forget to re-enable before exiting the REPL, I don't want to have to explicitly re-enable the next time I fire up the REPL. That is just a recipe for user confusion, as it forces them to remember what they did in the last session when starting a new session when certain REPL features no longer work.

It's possible that we could explicitly give users the option to "restore" the last session on startup (e.g., similar to how some web browsers, such as Chrome, may ask you whether you want to restore your last browser session, reopening tabs, etc).

[Snehil-Shah]

I get your use case but it's more likely that if the user disabled auto-closing, they just don't want it. And them having to change all settings to their preferences everytime they boot up the REPL seems more annoying/confusing as it's pretty standard (in IDEs or any application) that once they change a setting, they want it that way.
And if they were just experimenting and fiddling around, they can always just use the settings command to see what's enabled/disabled.

In short, it would be more confusing not saving by default as it would force a user to remember what they changed in the last session so they can change it back again to what they wanted everytime they enter the REPL.

[kgryte]

Why don't we avoid this discussion altogether and just prompt a user before exiting the REPL as to whether they want to save current REPL settings when the current REPL settings differ from what's on file? In short, punt the decision to the user.

[kgryte]

BTW: you'd still need a last_repl_session.json file to allow manual restore if a user's system crashes while the REPL is open and before settings and other session info could be persisted.

[kgryte]

In short, here is a possible summary:

1. On startup, compare the settings from the last REPL session with the "default" settings (i.e., a user's saved configuration). If these differ, ask the user whether they want to restore the previous session's settings. If yes, restore. If no, overwrite the last_repl_session.json file with the default settings.
2. During REPL usage, allow changing settings without persistence. Allow a user to explicitly run a command to save settings (e.g., via saveSettings or some other command).
3. On exit, if the current REPL settings differ from their default settings, prompt the user whether they want to save the REPL's current settings. It may be useful to show to the user what's changed. If the user says "yes", overwrite a user's default settings. If "no", don't save and exit the REPL. The user should also be able to hit Ctrl+C to return to the REPL (and not exit) to toggle settings, if the current mix of settings contains some changes they want to keep and others they don't and they want the opportunity to clean this up before trying to exit again.

[Snehil-Shah]

@kgryte I decided to let them just use a key binding while exiting if they want to save preferences. This way we don't add any additional steps that the user probably wouldn't appreciate. Maybe we can add a command as well saveAndQuit() analogous to quit()..?

And I think we can figure out restoring a session upon a crash when we work on being able to save the entire REPL state along with variables etc, is that fine?

[kgryte]

And I think we can figure out restoring a session upon a crash when we work on being able to save the entire REPL state along with variables etc, is that fine?

Yes, that is fine. So long as in the meantime we are saving the current session settings while a REPL session is underway to a file, such as last_repl_session.json. The restoration can happen in a follow-up PR. Would be good to open an RFC to this end on the issue tracker.

use a key binding while exiting if they want to save preferences

Maybe instead, and only when settings have actually changed during the current REPL session,

In [1]: <Ctrl+C>
Would you like to save the current REPL settings? [Y/N]
Settings saved.
To exit, enter ^D or ^C again or enter quit().

When no settings have changed,

In [1]: <Ctrl+C>
To exit, enter ^D or ^C again or enter quit().

When a user opts not to save settings.

In [1]: <Ctrl+C>
Would you like to save the current REPL settings? [Y/N]
To exit, enter ^D or ^C again or enter quit().

[kgryte]

I am not a fan of having multiple keybindings for exiting the REPL. Having two is almost too many. Having three just creates confusion.

[kgryte]

This way we don't add any additional steps that the user probably wouldn't appreciate.

Being explicitly prompted to save or affirming that one is okay navigating away from some page when changes have not been persisted is a common user interface pattern. Given that users are unlikely to be changing settings during every session, but once in a while, being explicitly prompted, without the additional cognitive overload of a separate keybinding, is a minor inconvenience at worst.

A REPL is not an IDE or a typical application. It's very essence is for doing ephemeral things. So we don't always need to treat it as an IDE where a user explicitly navigates to a settings panel or updates a specific configuration file, where it is assumed that changing a setting results in persistence. In a REPL, most user actions assume the very opposite. And I don't think we should assume that simply because a user changed a setting (e.g., a theme), that this automatically means they want to live with this change forever more (i.e., to persist). So having a separate explicit prompt, independent of exiting keybindings, seems a reasonable compromise and a minor speed bump.

[Snehil-Shah]

When the user is inside a "profile" meaning they have a custom configuration in any of their parent directories, and then the user saves when exiting, do we update their custom configuration or update the default ~/.stdlib/repl.json?

[kgryte]

a custom configuration in any of their parent directories

Good question. I think we need to remember the configuration file path of whatever configuration file was resolved and loaded on REPL startup. E.g., if we resolve a config in the current working directory, we should save to that file on exit.

But oof. That is tricky and could potentially lead to some unexpected behavior if people move around directories with local configs, but, similar to package.json, I think people can get used to the idea that settings changes are propagated to whatever config was loaded on startup.

[Snehil-Shah]

But what if the configuration was loaded from a .js export, where do we save it then? Would it be better to just stick to .json for configuration?

[kgryte]

what if the configuration was loaded from a .js export

Ugh. 🐉 's everywhere. Sure, *.json for now is fine. We can always revisit later wrt *.js support.

[Snehil-Shah]

and also, should we prompt them to save after the second confirmation?

To exit, enter ^D or ^C again or enter quit(). 
// and then after that
Would you like to save the current REPL settings? [Y/N]

Because there is a possibility they might not want to actually quit so it's better to confirm if they are, before prompting them if they want to save..?

[kgryte]

Because there is a possibility they might not want to actually quit so it's better to confirm if they are, before prompting them if they want to save..?

If they don't want to exit, they can also just hit N and then resume operation in the REPL. Or they can hit Y and it saves. Either way, I think it should be fine to do after the first prompt.

[Snehil-Shah]

I don't know, I think it would make things confusing and the user might think ^C is for saving and not exiting?
Because if we first show the prompt "do you want to save", and then they might think this is how you save. and might not know if it also exits after that. I think the confirmation to exit first would make things clear that "ok I am exiting the REPL, oh do I want to save? Yeah..". WDYT?

[kgryte]

Okay. This is something we can always change later.

[Snehil-Shah]

config.mp4

[kgryte]

@Snehil-Shah Is it possible to avoid the second In [3]: input prompt?

[Snehil-Shah]

That is there for a reason, as it's not confirmed yet if the user wants to quit or stay. The prompt is there if they want to type again, or they can use ctrl+c again to quit (and if applicable save)

[Planeshifter]

Currently, exiting the REPL via quit() will not prompt the user for whether they want to save their preferences. Since this command is not described as "force quit", this behavior seems unintuitive to me and I would argue that we should be consistent and have it behave the same as if I would exit via one of the other ways when greeted with:

To exit, enter ^D or ^C again or enter quit().

There is also no indication in the above message that ^D / ^C behave differently from typing quit().

In my view, the current behavior of quit() only makes sense if we had an additional saveAndQuit() command as suggested above. But personally, I think it's not too much of an inconvenience if one is prompted when exiting via quit() for whether one wants to save preferences, as this won't be the case that often.

PR otherwise looks good to me!

[Snehil-Shah]

@Planeshifter Agreed, the user should be prompted the same way when using the quit() command

[Snehil-Shah]

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)?

[kgryte]

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.

[Snehil-Shah]

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?