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: jotai/valtio #405

Merged
merged 6 commits into from
Apr 13, 2021
Merged

feat: jotai/valtio #405

merged 6 commits into from
Apr 13, 2021

Conversation

dai-shi
Copy link
Member

@dai-shi dai-shi commented Apr 2, 2021

This add a new bundle jotai/valtio with atomWithProxy.

  • impl
  • tests
  • docs

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 2, 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 bca497e:

Sandbox Source
React Configuration
React Typescript Configuration

@Thisen
Copy link
Collaborator

Thisen commented Apr 3, 2021

It's fun that you can do this, but it seems somewhat odd aswell. What's the benefits over using Immer integration?

I'm scared about the added confusion as well.

@dai-shi
Copy link
Member Author

dai-shi commented Apr 3, 2021

It's fun that you can do this, but it seems somewhat odd aswell. What's the benefits over using Immer integration?

Immer integration is just for updating atom values. This is to sync a value outside React.

I'm scared about the added confusion as well.

Any idea to mitigate it?

@Thisen
Copy link
Collaborator

Thisen commented Apr 3, 2021

I was comparing them on an impl level (proxies).

If you want to introduce sync to values outside React, I think we should consider adding another primitive than Valtio.

I'm scared about how it will affect adoption, because of search engines. People will need to read the docs to understand how to use Valtio inside Jotai.

I believe we should consider adding a primitive for syncing outside React using proxies. We can borrow ideas from Valtio.

Just my two cents.

@dai-shi
Copy link
Member Author

dai-shi commented Apr 3, 2021

Hope to have a better idea in docs. If it's too confusing to many people, I might consider releasing it as a 3rd-party package.

@sandren
Copy link
Collaborator

sandren commented Apr 3, 2021

I'm scared about how it will affect adoption, because of search engines. People will need to read the docs to understand how to use Valtio inside Jotai.

I agree. While it may make sense from a development perspective to use the already complete valtio/vanilla bundle for the proxy implementation, the name of the complementing Jotai bundle can easily introduce confusion.

Perhaps the new bundle should be called jotai/proxy instead of jotai/valtio? I think that makes sense since the feature it introduces is atomWithProxy, not atomWithValtio.

It may also seem odd to introduce a second state management library as an extra dependency in a project that already has another state management library for the purpose of supporting a single feature in the original state management library. Perhaps the valtio peer dependency could be replaced by a direct dependency within a new jotai/proxy bundle?

I think these changes would address everyone's concerns: no confusion for Jotai users, no duplicated code for Jotai contributors. 😀

@dai-shi
Copy link
Member Author

dai-shi commented Apr 3, 2021

two out of two is not many, but feels strong opinion.
so, I'm fine to release this in https://www.npmjs.com/package/jotai-valtio.

let me note, this is almost analogous to jotai/xstate. xstate is a state management library.

xstate valtio
what it is state management based on state machines state util with proxy
vanilla bundle xstate valtio/vanilla
react bundle @xstate/react valtio (vanilla combined)
jotai binding function atomWithMachine atomWithProxy
the goal of binding use machine config sync with proxy state

In terms of the goal, jotai/valtio is more close to jotai/query.
Maybe, the main issue is it's hard to explain "valid" use cases for jotai/valtio without misunderstanding.
In this sense, jotai/zustand would be confusing, but jotai/redux would not.

@Thisen
Copy link
Collaborator

Thisen commented Apr 3, 2021

As @sandren said, jotai/proxy would make more sense. Then the impl can be inspired by Valtio.

@dai-shi
Copy link
Member Author

dai-shi commented Apr 3, 2021

Note two things: a) it's not easy to create the same feature as valtio, and if I were to do it, it would be the same code. b) if it doesn't depend on valtio, it can just be in jotai/utils.

Anyway, it's not requested by anybody, and my goal is to connect with valtio which already has a clean api, so 3rd-party lib would be the way to go.

@dai-shi dai-shi closed this Apr 3, 2021
@sandren
Copy link
Collaborator

sandren commented Apr 3, 2021

Maybe, the main issue is it's hard to explain "valid" use cases for jotai/valtio without misunderstanding.

The use case would be for any project that needs read/write access to Jotai state outside of React. I think once people understand that, they will see many "valid" use cases for adding Valtio as an additional dependency to an existing React + Jotai codebase. 😀

@aulneau
Copy link
Collaborator

aulneau commented Apr 3, 2021

Just my 2 cents:

I am a huge fan of how jotai has these various integrations with other libs: xstate react-query immer soon rxjs, etc. I thought the idea of doing one with valtio made so much sense, and I think having these tight integrations with other really useful libraries can be a unique selling point of jotai. Sad to see this PR closed.

Around the confusion, imo it's totally a problem of education more than implementation.

@dai-shi
Copy link
Member Author

dai-shi commented Apr 3, 2021

Let's reopen it to have some more discussions.

Pinning @Aslemammad to get some opinions.

@dai-shi dai-shi reopened this Apr 3, 2021
@aulneau
Copy link
Collaborator

aulneau commented Apr 3, 2021

One other thing -- I think it's really easy to say "people could be confused by this" but I think a better indicator is if folks make issues saying "i am confused by this." It's easy to over optimize for a problem that doesn't exist yet.

@sandren
Copy link
Collaborator

sandren commented Apr 3, 2021

One other thing -- I think it's really easy to say "people could be confused by this" but I think a better indicator is if folks make issues saying "i am confused by this." It's easy to over optimize for a problem that doesn't exist yet.

After I read @dai-shi's comment explaining the reasoning behind the jotai/valtio name, it totally made sense to me.

If people can understand import { atomWithMachine } from 'jotai/xstate', then they can understand import { atomWithProxy } from 'jotai/valtio'. We should give them more credit.

In general I think the risk of a few people being confused at first is not worth abandoning what could turn out to be a really amazing feature. I'm happy to see the PR re-opened under the original name of jotai/valtio. 😀

@vercel
Copy link

vercel bot commented Apr 5, 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/913E86TjUaog8NbJRyPXFyXL6cjp
✅ Preview: https://jotai-git-feat-valtio-pmndrs.vercel.app

@dai-shi
Copy link
Member Author

dai-shi commented Apr 13, 2021

Merging this as we want to move forward. Our discussions here are valuable. Suggestions to improve docs are always welcome.

@dai-shi dai-shi merged commit 0e01c87 into master Apr 13, 2021
@dai-shi dai-shi deleted the feat/valtio branch April 13, 2021 01:56
@dai-shi dai-shi mentioned this pull request Apr 13, 2021
49 tasks
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.

4 participants