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

Update lib/local-storage.js to ES2015 #242

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

Nathan-Schwartz
Copy link
Contributor

Tests and linter pass. Runs in Node without babel.

jbrumwell and others added 23 commits September 23, 2016 17:17
Conflicts:
	dist/command.js
	dist/option.js
	lib/command.js
Updated dev deps and switched to Yarn
Updated Inquirer to v3 and other prod deps
…values

Support default values for options
Update babel/eslint build config
@milesj
Copy link
Contributor

milesj commented May 31, 2017

@Nathan-Schwartz @dthree

I feel like this entire file can be removed. It's almost a 1:1 map of the native LocalStorage, with the addition of validate(). We can easily move all this logic into Vorpal#localStorage.

For example, this is the current code:

vorpal.localStorage = function (id) {
  var ls = Object.create(LocalStorage);
  ls.setId(id);
  _.extend(this.localStorage, ls);
  return this;
};

Which can be changed to:

vorpal.localStorage = function (id) {
  if (!id) {
    throw new Error('vorpal.localStorage() requires a unique key to be passed in.');
  }

  Object.assign(this.localStorage, new LocalStorage(DEFAULT_STORAGE_PATH + id));

  return this;
};

I haven't tested it yet, but worth a shot.

@dthree
Copy link
Owner

dthree commented Jun 1, 2017

Agreed. Go for it.

@dthree dthree closed this Jun 1, 2017
@dthree dthree reopened this Jun 1, 2017
@Nathan-Schwartz
Copy link
Contributor Author

Sounds good to me. Seems like we can close this.

@milesj Would you like to take a crack at reusing Vorpal#localStorage? Not sure what kind of time I'll have in the near future.

@milesj
Copy link
Contributor

milesj commented Jun 1, 2017

Yeah I can take a look at it. I'll leave this PR open in case it doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants