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

localStorage support. #77

Merged
merged 4 commits into from
Jan 23, 2015
Merged

localStorage support. #77

merged 4 commits into from
Jan 23, 2015

Conversation

hemanth
Copy link
Contributor

@hemanth hemanth commented Jan 21, 2015

@thejameskyle First cut. Let me know if this approach is OK with you. WRT #71

@sebmck
Copy link
Contributor

sebmck commented Jan 21, 2015

A check for the existence of localStorage would be good to support browsers that don't have it.

@hemanth
Copy link
Contributor Author

hemanth commented Jan 21, 2015

@sebmck Was wondering the same, looks like only Opera mini doesn't have it, yet!

@hemanth
Copy link
Contributor Author

hemanth commented Jan 21, 2015

@sebmck Added a MayBe construct, what say ;) ?

@sebmck
Copy link
Contributor

sebmck commented Jan 21, 2015

@hemanth Bit overkill. I was suggesting more if (window.localStorage) localStorage.setItem("foo", "bar");. Your solution wont work either because the localStorage reference will throw a ReferenceError because it's not declared.

@hemanth
Copy link
Contributor Author

hemanth commented Jan 21, 2015

@sebmck Duh! yeah, I was overwhelmed by mayBe, heh heh, reverting it to a simpler version.

@hemanth
Copy link
Contributor Author

hemanth commented Jan 21, 2015

@sebmck Done 🐱

@jamiebuilds
Copy link
Contributor

@hemanth Could you actually clean up these commits by rebasing your commit onto the latest master?

@sparty02
Copy link
Contributor

@hemanth I've found this wiki to be useful in respect to rebasing pull requests , and squashing if need be

@hemanth
Copy link
Contributor Author

hemanth commented Jan 21, 2015

@sparty02 Thanks, i'm aware of it, I fetched and merged your branch, hence the older commits have appeared.

@hemanth hemanth changed the title localStorage first cut localStorage support. Jan 21, 2015
@hemanth
Copy link
Contributor Author

hemanth commented Jan 21, 2015

@thejameskyle Done.

@@ -109,6 +109,10 @@
var state = UriUtils.parseQuery();
this.options = _.assign(new Options(), state);

if (window.localStorage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@hemanth You may want to move the local storage call up above where options are set on the repl instance:

this.options = _.assign(new Options(), state);

(so that options can be persisted too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@sparty02 Done :)

@sparty02
Copy link
Contributor

Local storage is an interesting idea, but I wonder if it would be confusing at all if a user came back to the site and there was all of a sudden input (via local storage) in their REPL. I definitely think persisting options is nice, I'm just concerned about how to effectively persist the code.

I'm almost wondering if this should be beefed up into some kind of "save session for later" feature (which could key into local storage by ID). This could then be pulled up via a menu.

Note, if it was taken this far, this could tie into #59 (via how to seed the REPL input from a dropdown) and #66 (the idea of 'saving' a session for later in a more robust way)

Just some thoughts......

@hemanth
Copy link
Contributor Author

hemanth commented Jan 21, 2015

@sparty02 I would vote to get this rolling first and improvise later?

I have come across few online REPLs that save the previous code as is and is not confusing, maybe we must roll and wait for feedback from users?

@gaearon
Copy link
Member

gaearon commented Jan 21, 2015

Note localStorage.setItem and localStorage.getItem throw on iOS in Private Mode.
Not that it's very important to support REPL on iOS, but not nice to crash either.

@jamiebuilds
Copy link
Contributor

Note localStorage.setItem and localStorage.getItem throw on iOS in Private Mode.
Not that it's very important to support REPL on iOS, but not nice to crash either.

It's probably fine to just wrap these in try-catch blocks. @hemanth mind doing that?

@gaearon
Copy link
Member

gaearon commented Jan 21, 2015

It's probably fine to just wrap these in try-catch blocks.

This is what I usually do, yeah.

@hemanth
Copy link
Contributor Author

hemanth commented Jan 22, 2015

@thejameskyle No other options left, I had created a MayBe monad :D which was an overkill :)

@jamiebuilds
Copy link
Contributor

I would just leave it as a try-catch

try {
  state = JSON.parse(localStorage.getItem('replState'));
} catch(e) {}

@hemanth
Copy link
Contributor Author

hemanth commented Jan 22, 2015

@thejameskyle Done.

@hemanth
Copy link
Contributor Author

hemanth commented Jan 23, 2015

@thejameskyle @sebmck 🐱

jamiebuilds added a commit that referenced this pull request Jan 23, 2015
@jamiebuilds jamiebuilds merged commit f52cf29 into babel:master Jan 23, 2015
@jamiebuilds
Copy link
Contributor

Thanks!

@hemanth
Copy link
Contributor Author

hemanth commented Jan 23, 2015

Thank you!

@hemanth hemanth deleted the local-storage branch January 23, 2015 05:12
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.

5 participants