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

feat(utils): Add loadable #698

Merged
merged 1 commit into from
Sep 13, 2021
Merged

feat(utils): Add loadable #698

merged 1 commit into from
Sep 13, 2021

Conversation

Pinpickle
Copy link
Contributor

Fixes: #672 (or at least tries to)

This implements loadable as discussed in the mentioned issue.

I can't get all edge-cases to work. In particular, recovering from error, when an atom goes through these states:

suspense -> error -> suspense -> resolved

Going from error -> suspense does not trigger a re-render.

I think this might need some changes to core, as I suspect by catching the error/promise, we are preventing some state-keeping logic from happening.

@dai-shi could you have a look? It's the last test in loadable.test.tsx that's failing.

@vercel
Copy link

vercel bot commented Sep 9, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/jotai/7vC8KaT7kiUF2X1EP16WUV92zd1o
✅ Preview: https://jotai-git-fork-pinpickle-add-loadable-pmndrs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 9, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 15fbff3:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
Next.js Configuration

@dai-shi
Copy link
Member

dai-shi commented Sep 10, 2021

Nice work.
I have some coding preference to apply, reducing bundle size, fixing refAtom usage, but in general, it looks good.

However:

I can't get all edge-cases to work. In particular, recovering from error

Oh, right... This is not resolved yet, if it's same as #601.
Yeah, I agree it's probably core responsibility.
I think it's going to take some time for this.
Should we fix it first, or could loadable be useful with this limitation for now?

@Pinpickle
Copy link
Contributor Author

@dai-shi I wasn't aware that it was a bug for all atoms

Given that this is a bug with all atoms, I think it's OK to release as-is. It should probably be documented somewhere that error recovery doesn't work. Should I mark the test as skipped or remove altogether?

@dai-shi
Copy link
Member

dai-shi commented Sep 12, 2021

I'm not 100% sure if it's the same bug as #601. (tbh, I haven't spent much time on it yet.)

Please leave the tests as skipped. Let's tackle it separately, if this is useful as is.
I'll request some changes on code.

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

My final recommendation is to use WeakMap instead of refAtom, but let me shamelessly leave my intermediate comments.

src/utils/loadable.ts Outdated Show resolved Hide resolved
src/utils/loadable.ts Outdated Show resolved Hide resolved
src/utils/loadable.ts Outdated Show resolved Hide resolved
@Pinpickle
Copy link
Contributor Author

@dai-shi I've addressed your comments, it's a lot cleaner now.

Let me know what you think.

I also looked into #601 and it is a core bug, opened a PR at #705

@Pinpickle Pinpickle marked this pull request as ready for review September 12, 2021 20:59
@Pinpickle Pinpickle changed the title wip: Add loadable Add loadable Sep 12, 2021
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Looks very nice!
Added a small suggestion to refactor.

src/utils/loadable.ts Outdated Show resolved Hide resolved
src/utils/loadable.ts Outdated Show resolved Hide resolved
src/utils/loadable.ts Outdated Show resolved Hide resolved
@Pinpickle
Copy link
Contributor Author

@dai-shi done! I did the same for the loadable cache as well, it's only one key

await findByText('Error: An error occurred')
})

it('loadable turns primitive throws into values', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we're using a WeakMap, I added this test case. If a primitive value ever became a key for error, it wouldn't work.

@dai-shi
Copy link
Member

dai-shi commented Sep 12, 2021

I did the same for the loadable cache as well, it's only one key

Interestingly, I didn't notice it. Thanks. (There might be other utils we could do the same.)

src/utils/loadable.ts Outdated Show resolved Hide resolved
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks so much.

@dai-shi dai-shi merged commit c4a9962 into pmndrs:main Sep 13, 2021
@dai-shi
Copy link
Member

dai-shi commented Sep 13, 2021

Would you also work on docs? For now, docs are in a separate repo for now.
https://github.com/pmndrs/website/tree/docs/docs/jotai/api/utils.mdx
(and, we should probably split utils page in to some pages.

@Pinpickle
Copy link
Contributor Author

@dai-shi yep I can have a look at that, will probably be next week

@TwistedMinda
Copy link
Collaborator

TwistedMinda commented May 21, 2022

EDIT: Moving my comment in the open discussion instead
EDIT2: Not sure it was the best move

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.

loadable API for using async atoms without suspense
3 participants