Lucid-Toys / chrome-lucid

✍️ A chrome extension for replacing your new tab page with a very simple notepad.
https://chrome.google.com/webstore/detail/lucid/achogfadpkcepkepcpegehpiiioihmik?hl=en
82 stars 21 forks source link

Switched localStorage to chrome.storage.sync #3

Closed ghost closed 7 years ago

ghost commented 7 years ago

This is the first time I'm doing a pull request, so hopefully everything is in order.

I wanted to add support for multiple devices by storing the data in chrome.storage.sync instead of localStorage, so it syncs across all devices that the account is attached to.

In order for this to work I had to add the storage permission.

I actually did a pull request earlier today with this functionality before the refactor commits.

Once I realized I was behind, and that everything had been changed, I pulled upstream back in, scrapped the changes I had made, and made them again in a new way to match the latest version. At this time I also noticed an issue with the fallback for before v0.0.3, as the try never failed and the catch never ran, since data was just being set to null. In this update I also made sure to add working fallback code that accounts for just the content being set in local storage, and the object being set in local storage. It recognizes both of these situations and converts them over to the new object in chrome sync storage.

Since chrome.storage.sync is asynchronous, I had to change the readStore() function to use a callback. Since we are relying on this callback, I moved the bulk of the code into a start() function.

I was about to submit my pull request, then I realized another push had been made for the tab sync support. I pulled that in and updated the new code to use the new readStore function.

Let me know if there is anything else I should do here. Hopefully the multiple commits + merging in the upstream twice doesn't make it too difficult to parse.

Great job on this by the way, I liked it a lot and wanted to help make it even more useful :)

ghost commented 7 years ago

Hey there,

Sorry for the delay, things got quite busy at work. I'm sure you know how it goes :)

The code I wrote should handle converting over the object in local storage.

To test this I added the following just below the defaultData declaration on line 106:

let testData = defaultData
    testData.notepadContent = "TESTING"
chrome.storage.sync.clear()
localStorage.setItem(key,JSON.stringify(testData))

This sets some testing data as an object in local storage, and clears the chrome sync storage. The fallback code should then pull in the "TESTING" content and convert it over to chrome.storage.sync.

I have added some new comments in the fallback section explaining exactly what it's done, as it was a bit confusing when I was looking over it again.

I also pulled in the upstream again and merged. You had added some new things that were specifically for handling localStorage, but since we aren't doing that anymore, I basically just had to remove the conflicting code.

I also updated the version to 1.1.2, since you've made some improvements since, and that would be the next version number.

Let me know if you run into any more issues, or if that test doesn't work for you.

Thanks!

daneden commented 7 years ago

@dustindikes awesome, thank you for the contribution!