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

Allowing a bit of flexibility for libraries / abstractions above recoil #451

Closed
natew opened this issue Jul 8, 2020 · 6 comments
Closed

Comments

@natew
Copy link

natew commented Jul 8, 2020

So, I tried opening both an issue and a PR, but both were closed. The issue first, and then the PR was closed as the closing person had requested I open an issue first.

I was given very positive (thank you!) feedback from @drarmstr and @davidmccabe on the issue, and I should have referred to it in the PR. My mistake, a lot is going on outside me opening these issues, so I had forgotten I had even opened the issue first.

Basically, all I'd need to make this issue workable, is simple. It would be to expose the internal useRecoilInterface method. I'd totally support that method being exposed in some very non-intuitive way (eg, behind an obscure name) as it would only be for a pretty spare use case: library authors.

Here is the PR that exposes it, as you can see quite short.

I'd like to ask @mondaychen to reconsider, and open a discussion on the feasibility. As I understand it, you all may feel uncomfortable as the underlying way recoil works may change wholesale. Understanding how you plan to change that would be helpful for me.

The primary need for using useRecoilInterface is basically that it allows me to make a call once to get the instance, and then later on access more than one atom/selector during render. If I was to try and implement this with the currently exposed interfaces, I'd immediately hit the classic hooks issue of accessing different amounts of hooks across renders. That is really the only driving force behind it.

To be honest, I've wanted to build a store system like this for years and years (unfortunately, the old video we made showing our prototype doesn't show the store system, but we had a store system that basically was just a class interface that you could use inside any view). Recoil also has in my mind resolved one of the last niggles I had with it, which was how to elegantly have multiple stores. I wont go into that so as to not muddy this issue, but basically I appreciate the work you've done.

I've thought about implementing this outside of Recoil entirely, and that's a valid concern. Perhaps the right way to do it. But being of limited time, and wanting to support concurrent mode in the future, and generally wanting to ride on the backs of giants and not have to worry about the many many details of building a state system, I was incredibly pleased when I found I was able to get the store system working with <200 lines of code on top of Recoil. If anything, supporting it is a real testament to the well doneness of the API you've built.

Look forward to hearing any feedback, in the case you want to wait for the refactor you've alluded to before making any decisions, that is fine, I'm happy to keep my local patch alive until then.

@drarmstr
Copy link
Contributor

drarmstr commented Jul 8, 2020

If you want to explore and hack around with dynamic calls that bypass the ordering and non-conditional restrictions of hook calls, will the following work for now?

  const setRecoilState = useRecoilCallback(({set}) => (recoilState, value) => set(recoilState, value), []);
  const getRecoilValue = useRecoilCallback(({getLoadable}) => recoilValue => getLoadable(recoilValue).getValue(), []);

@natew
Copy link
Author

natew commented Jul 9, 2020

That seems possible, I think actually one of my main questions was... is the set and get values that are passed around specific in some way. They are passed into every atom/selector as an argument, and without looking deeply into the internals I wasn't sure if they were custom to the selector/atom or were generic. Knowing that I can re-use set/get like that is useful, I will give it a shot.

Edit: just to be sure, the getLoadable doesn't exist as of 0.10.0, right?

@drarmstr
Copy link
Contributor

drarmstr commented Jul 9, 2020

The get() and set() callbacks provided to selectors are specific for that selector and should not be passed around. The workaround above uses useRecoilCallback() which is intended to create a callback to use Recoil functionality and that callback can be passed and used asynchronously. useRecoilCallback() does not have a get(), but has a getLoadable() and getPromise() currently.

@natew
Copy link
Author

natew commented Jul 14, 2020

Ah ok, for some reason I wasn't seeing it.

image

Is it not .snapshot.getLoadable?

I should just go back and test it again...

@drarmstr
Copy link
Contributor

Is it not .snapshot.getLoadable?

Yes, Snapshot has the getLoadable() accessor.

@salvoravida
Copy link
Contributor

may be related
#829

@natew natew closed this as completed Jan 13, 2021
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

No branches or pull requests

3 participants