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

More examples of Local Storage Persistence #828

Merged
merged 4 commits into from Feb 3, 2021
Merged

More examples of Local Storage Persistence #828

merged 4 commits into from Feb 3, 2021

Conversation

mthines
Copy link

@mthines mthines commented Jan 12, 2021

Asynchronous / Promise examples of Local Storage Persistence

#767

Asynchronous / Promise examples of Local Storage Persistence
@facebook-github-bot
Copy link
Contributor

Hi @mthines!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@mthines
Copy link
Author

mthines commented Jan 12, 2021

I've now signed the agreement. 🙂

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 12, 2021
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Copy link
Contributor

@drarmstr drarmstr left a comment

Choose a reason for hiding this comment

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

Thanks @mthines for helping to improve our documentation!!

@@ -239,6 +241,80 @@ const currentUserIDState = atom({
});
```

### Asynchronous

If your persisted data needs to be `await`ed, you can either create an `asynchronous` function and call that or use a `promise` in the setSelf function
Copy link
Contributor

Choose a reason for hiding this comment

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

promise -> Promise
setSelf -> setSelf()

@@ -239,6 +241,80 @@ const currentUserIDState = atom({
});
```

### Asynchronous
Copy link
Contributor

Choose a reason for hiding this comment

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

localStorage is only sync afaik. For async examples let's move out of the "Local Storage" section into another subsection and not reference localStorage.

Copy link
Author

Choose a reason for hiding this comment

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

That's very true. My original code was using localForage which is async, so it made sense in that case, but I converted that part to localForage since the other framework was related to my project and not recoil.

I'm uncertain how to write the documentation for this part then. Any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Split into different "##" level sections.

  • Keep Local Storage Persistence as-is, maybe a mention that it is synchronous in its description.
  • Add another section for Asynchronous Storage Persistence mentioning maybe AsyncLocalStorage and localForage as examples.

```jsx

/** If there's a persisted value - set it on load */
const loadPersisted = async ({ key, setSelf }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Passing through the setSelf() callback might be confusing. To help with clarity for documentation purposes maybe define this async function in the effect function so it can just use key and setSelf() directly.

Copy link
Author

Choose a reason for hiding this comment

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

With documentation in mind, that makes sense. I'll do that.


/** If there's a persisted value - set it on load */
const loadPersisted = async ({ key, setSelf }) => {
const savedValue = await localStorage.getItem(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

See above comment re localStorage being sync

docs/docs/guides/atom-effects.md Show resolved Hide resolved
```


#### Async Await
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach isn't limited to just await, but for any async usage of setSelf(), such as with setTimeout().

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the "asynchronously call setSelf()" approach to again be more explicit.

Mention that the default atom value will be used until the deferred setSelf() is called to set the value.

@drarmstr drarmstr added the documentation Improvements or additions to documentation label Jan 20, 2021
@drarmstr drarmstr self-assigned this Jan 20, 2021
@@ -239,6 +241,80 @@ const currentUserIDState = atom({
});
```

### Asynchronous
Copy link
Contributor

Choose a reason for hiding this comment

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

Split into different "##" level sections.

  • Keep Local Storage Persistence as-is, maybe a mention that it is synchronous in its description.
  • Add another section for Asynchronous Storage Persistence mentioning maybe AsyncLocalStorage and localForage as examples.


If your persisted data needs to be `await`ed, you can either create an `asynchronous` function and call that or use a `promise` in the setSelf function

- Using the promise approach you'll be able to wrap the components inside of the `<RecoilRoot/>` with a `<Suspense/>` component to show a fallback state while waiting for `Recoil` to load the persisted values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not call this the "promise approach", as Promises may be used in other approaches as well. Maybe "Synchronously call setSelf() with a Promise". Wordy, perhaps, but more explicit.

Copy link
Author

Choose a reason for hiding this comment

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

Haha, I prefer it being more readable and explicit.

docs/docs/guides/atom-effects.md Show resolved Hide resolved
```


#### Async Await
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the "asynchronously call setSelf()" approach to again be more explicit.

Mention that the default atom value will be used until the deferred setSelf() is called to set the value.

docs/docs/guides/atom-effects.md Show resolved Hide resolved
@mthines
Copy link
Author

mthines commented Jan 27, 2021

Hey @drarmstr!

Sorry for taking so long getting back to this. I've had a crazy busy week 🏃

I've updated the atom-effects.md based on the changes and suggestions you had.
Let me know if I've missed anything, or somethings still unclear.

@mthines mthines requested a review from drarmstr January 27, 2021 19:48
Copy link
Contributor

@drarmstr drarmstr left a comment

Choose a reason for hiding this comment

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

Thank you so much for iterating on this! I think this should be the last round.

```jsx
const localStorageEffect = key => ({setSelf, onSet}) => {
setSelf(
localStorage.getItem(key).then((savedValue) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

localStorage only stores UTF-16. Best to avoid localStorage completely for the async examples. You could also just use localForage for this:

setSelf(localForage.getItem(key).then(savedValue =>
  savedValue != null
    ? JSON.parse(savedValue)
    : new DefaultValue() // Abort initialization if no value was stored
));

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, that's my bad.
We already talked about this, so simply a typo.
Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Resolved.


By synchronously calling `setSelf()` with a `Promise` you'll be able to wrap the components inside of the `<RecoilRoot/>` with a `<Suspense/>` component to show a fallback while waiting for `Recoil` to load the persisted values.

The `<Suspense/>` fallback will be shown until the `setSelf()` function has been called.
Copy link
Contributor

Choose a reason for hiding this comment

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

"<Suspense> will show a fallback until the Promise provided to setSelf() resolves."

Copy link
Author

Choose a reason for hiding this comment

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

Resolved.

};

// Load the persisted data
loadPersisted()
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing semi-colon.

Copy link
Author

Choose a reason for hiding this comment

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

Resolved.


Using an asynchronous function your `atoms`, `selectors` etc, will use their default value until the deferred `setSelf()` has been called to set the value.

Unlike using a `Promise`, it is not possible to use `<Suspense/>` to show a fallback.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Unlike initializing to a Promise, the atom's default value will be used initially, so <Suspense> will not show a fallback unless the atom's default is a Promise or async selector."

Copy link
Author

Choose a reason for hiding this comment

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

Resolved.

@drarmstr drarmstr merged commit fc47eff into facebookexperimental:docs Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants