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

RFC: Hoisted Stores #241

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

RFC: Hoisted Stores #241

wants to merge 7 commits into from

Conversation

rcharmeyer
Copy link

@rcharmeyer rcharmeyer commented Feb 15, 2023

NOTE: This RFC is stale, I'm not maintaining it due to lack of interest.

View RFC

Since the beginning, the API has changed to createStore and createScope.

This is a little more manual but it's more flexible and clear than the original hoist proposal was.

Quick Example

const CounterScope = createScope ()

const countStore = createStore (() => {
  const [ count, setCount ] = useState (0)
  return { count, setCount }
}, [ CounterScope ])

function Counter () {
  const { count, setCount } = use (countStore)
  // ...
}

const App = () => (
  <CounterScope>
    <Counter />
    <Counter />
  </CounterScope>
)

What was the original hoist proposal?

hoist was effectively around createStoreFamily which itself is a wrapper around createStore.

The one difference in createStore was that instead of having createScope, the original proposal relied on useContext to determine the scope. This approach worked except when the context provided to useContext changes or when conditional hooks were used.

const CounterContext = createContext ()

const useCountStore = hoist (() => {
  useContext (CounterContext)
  const [ count, setCount ] = useState (0)
  return { count, setCount }
})

function Counter () {
  const { count, setCount } = use (countStore)
  // ...
}

const App = () => (
  <CounterContext.Provider>
    <Counter />
    <Counter />
  </CounterContext.Provider>
)

Old Notes

CodeSandbox

I've added a CodeSandbox where I'm (roughly) implementing it as a library by hijacking the dispatcher and using Jotai under the hood. The sandbox is now stable and should support most of the original hooks as well as Suspense via the makeAsync utility.

@lubieowoce
Copy link

lubieowoce commented Feb 18, 2023

as someone unfamiliar with your idea of "hoisting", the first example doesn't really make it clear why we couldn't just stick the expanded-state into a regular context:

const ExpandedCtx = createContext(null);

// nothing fancy, just some state in a context

const ExpandedState = ({ children }) => {
  const [expanded, setExpanded] = useState(false);
  return (
    <ExpandedCtx.Provider value={[expanded, setExpanded]}>
      {children}
    </ExpandedCtx.Provider>
  );
}

const useExpandedState = () => useContext(ExpandedCtx);

used like so:

export function List({ ids }) {
  return <>
    {ids.map(id => (
      <IdContext.Provider key={id} value={id}>
        <ExpandedState> // <-- added
          <Item />
        </ExpandedState>
      </IdContext.Provider>
    ))}
  </>
}

// the rest is unchanged

function ExpandedContent() {
  const [ expanded ] = useExpandedState()
  if (!expanded) return null
  // ...
}

function ExpandButton() {
  const [ expanded, setExpanded ] = useExpandedState()
  // ...
}

function Item() {
  return <>
    <ExpandButton />
    <Content />
    <ExpandedContent />
  </>
}

are the semantics you're proposing for hoist() different from this somehow? if so, why is that useful? could you explain a bit more about what your version allows that this doesn't, or what possibilities it opens up?

another thing that muddles the waters for me is IdContext itself -- are the unique id's somehow important to the example? or is that just an example context someone might have? if it's the former, it needs more explanation about how IDs play into this. if it's the latter, I'd consider making it, like, ColorContext or something instead to avoid confusing things.

@rcharmeyer
Copy link
Author

as someone unfamiliar with your idea of "hoisting", the first example doesn't really make it clear why we couldn't just stick the expanded-state into a regular context:

...

are the semantics you're proposing for hoist() different from this somehow? if so, why is that useful? could you explain a bit more about what your version allows that this doesn't, or what possibilities it opens up?

another thing that muddles the waters for me is IdContext itself -- are the unique id's somehow important to the example? or is that just an example context someone might have? if it's the former, it needs more explanation about how IDs play into this. if it's the latter, I'd consider making it, like, ColorContext or something instead to avoid confusing things.

Thanks for the feedback! So for the basic example I tried to use a minimal example that gives some idea of how hoist would behave instead of focusing on why it's desirable. I then added a much larger example at the bottom which better showcases the proposed API and I think that's where it starts to show why it's unique and valuable.

You're understanding is correct for the "Hoisting to Item-Level" example, we could absolutely use Context instead and, with the information given, we're not suffering for it. But there are use cases where this pattern falls apart. The "Hoisting to Global-Level" example is one of them. There is no direct Context-based equivalent for this and you're likely leveraging useReducer at that point to manage all of the expanded states.

@lubieowoce
Copy link

lubieowoce commented Feb 18, 2023

As far as I can tell, the longer example currently contains no usages of hoist(), so it doesn't help a lot. And it could use a description of what it's actually trying to do and some comments highlighting the important parts/behavior -- it's a lot of code, i don't know which parts are important to look at. in general i think this RFC is missing a concise "elevator pitch" saying what and why.

Some more questions that I didn't feel were explained very well or at all:

Scoping

how does hoist() know "how far up" it should go? does it look for whatever nearest context provider it can find up the tree and attach to that? A diagram would be useful

Conflicting hoisted calls from sibling components

what happens if two components within the same "hoist scope" (i think you use a name like that somewhere) do things like this

// <Foo />
useMyHoistedState(true);

// <Bar />
useMyHoistedState(false); // oops, different initial value

I'm not even sure what SHOULD happen here, or if there's a good answer at all.

Why not in userspace?

what's undesirable about your createScope implementation? that's the closest the RFC comes to a detailed explanation of the semantics, but honestly it looks like it'd work as is, so i think the RFC needs a better explanation for why that's not satisfactory and warrants special support in the react core. More words, less code ;) I'm assuming the downsides are obvious to you, since this is obviously a problem you've tried solving before. But readers don't have that context. I'm assuming lazy-loading and microfrontends also play into it somehow, since you've made vague gestures towards both, but again -- i don't KNOW why those introduce a challenge in this specific case.

@rcharmeyer
Copy link
Author

Thank you for all your feedback, it's tremendously helpful and I'm definitely going to be making some revisions. There are answers/reasons to all these points so I just need to find a way to communicate them.

I've tried to lean in to code examples to try and ground the conversation in something more objective. I've found it's been a difficult concept to explain with words and I think the examples are helping, this has gone better than most of my attempts to explain it verbally.

I'm starting to think this feature may be too dense to try and describe in a single document and am leaning towards an approach where the RFC is short and concise and that I provide supplementary material that would allow me to present the proposal in a way that's more thorough and digestible. Do you think that might be more valuable?

A couple responses:

As far as I can tell, the longer example currently contains no usages of hoist(), so it doesn't help a lot.

The last section of the Full Example (Built-In API) has the hoist-based implementation under. It is quite short.

how does hoist() know "how far up" it should go? does it look for whatever nearest context provider it can find up the tree and attach to that?

Correct, it uses the lowest Context.Provider of any Context which has been useContext-ed inside of it.

what happens if two components within the same "hoist scope" do things like this

The parameters to a "hoisted" hook, are like keys, they differentiate one "hoisted store" from another. So in your example, they aren't sharing state.

If you're familiar with Recoil or Jotai, without parameters a hoisted hook is kind of like an atom but with parameters it's like an atomFamily.

I have thought about calling the API createStore() and createStoreFamily(key) instead of hoist for this reason. You're certainly not the first person to interpret it this way and maybe the API should be named something else.

Why not in userspace?

react-hoisting does indeed work and I'm going to open-source it soon. hoist is better because of use cases involving lazy loading, lists and errors/suspense. Addressing those use cases in userspace isn't possible without giving up hooks.

@lubieowoce
Copy link

lubieowoce commented Feb 18, 2023

[hoist] uses the lowest Context.Provider of any Context which has been useContext-ed inside of it.

TBH i think re-using Contexts as hoist-scopes is gonna be a non-starter. Because then, if you suddenly want/need to add an unrelated context around a component, you can't -- that would break the hoisting! And it'd also make hoist() basically unusable in libraries w/o imposing significant restrictions on user code. So all in all i think an explicit createScope thing would be a better way to go. then, you'd have to something like hoist(My scope)(() => { /* hook */ })

@rcharmeyer
Copy link
Author

rcharmeyer commented Feb 18, 2023

if you suddenly want/need to add an unrelated context around a component, you can't -- that would break the hoisting!

Nope, only the relevant contexts matter! Adding another context doesn't break the hoisting.

it'd also make hoist() basically unusable in libraries w/o imposing significant restrictions on user code

This is really designed for libraries (or plugins, micro-frontends, etc). Code that can't make many implicit assumptions. I know it seems like it wouldn't work but I have yet to find a use case where it didn't work how I expected it too. (perhaps I'm just not seeing it though)

There are some caveats to using Context with hoist but they aren't that unexpected or burdensome for the end user.

TBH i think re-using Contexts as hoist-scopes is gonna be a non-starter.

I know the Context as the "hoist scope" thing is weird but it's actually what makes this work. The "hoisted store" (the instance inside React) is created/read from based on it's data flow dependencies. The key hypothesis behind the API is that Context is enough to determine the "appropriate" level of hoisting for a hook, that it will always be true and it will kind of just work.

Maybe a new kind of Context would be most appropriate and then the current Context is banned from use in hoist which would be terrible for adoption but if that's what's needed then so be it. I had an idea called KeyContext which is just a more restrictive Context like <MyKeyContext.Provider key={myKey}> and const myKey = useKeyContext (MyKeyContext).

@lubieowoce
Copy link

lubieowoce commented Feb 18, 2023

If we accept the definition of hoist(hook) as

  • works as though you ran the hook on the scope and passed the result down
    -in the child (consumer), the value passed to the hoisted hook "determines identity", i.e. if two descendants of a scope call the hook with the same args (===), they receive the same return value. so really it's as though the scope had a Map of hook invocations, keyed by the arg

then i see some potential challenges around effects:

  • when do we run effect cleanups? is it when the last descendant (for a given key) unmounts? that sounds like it could break some expectations, but i'm not sure. but I'd guess the scope would need to store a refcount for each key or something?
  • if all descendants for a given key unmount in one render, but then we get a new descendant using the same key, does the hook "reset"? if it does, that sounds like a pitfall... and if not, we can never clean anything up and get a source of memory leaks
  • could this get us into effect ordering issues? i.e. normally child effects run before parent effects. but if a child being present or not determines whether an effect "exists" on the scope, i feel like something weird might happen... maybe not, but just a hunch

and do we NEED this to handle effects and arbitrary hooks at all? maybe having a keyed updatable store would be enough? something like

// like `hoist((key) => useState(...))`
const PostStore = createKeyedState((key) => myInitialStateFromKey(key))

const Post = ({ postId }) => {
  const [state, setState] = useKeyedState(PostStore, postId);
  // ...
}

const PostTeaser = /* similar to the above, also accesses the store */

// we'd probably want some args for the provider too,
// but I'm skipping that for now
<PostStore.Provider>
  <Post postId={1} />
  <PostTeaser postId={1} /> // will share state with the first one 
  <Post postId={2} /> // separate state entry
</PostStore.Provider> 

...and if this isn't enough, what usecases is this missing? just a pointer for what the RFC could discuss ;)

@lubieowoce
Copy link

lubieowoce commented Feb 18, 2023

Nope, only the relevant contexts matter!

The key hypothesis behind the API is that Context is enough to determine the "appropriate" level of hoisting for a hook

so this mechanism would, what, pick the "outermost" context accessed within the hoist()ed hook and use that as a scope? that still sounds super brittle tbh. say, if someone goes and modifies a hoisted hook to access a ThemeProvider or react-query provider or anything that's "global" for the whole app. then your hooks could accidentally get pulled up much higher than intended, and debugging that doesn't sound like fun...

(and if you go for the "innermost" one, it'd still have the "oops adding new contexts might break things" issue i mentioned before)

@rcharmeyer
Copy link
Author

rcharmeyer commented Feb 19, 2023

(and if you go for the "innermost" one, it'd still have the "oops adding new contexts might break things" issue i mentioned before)

Definitely inner-most, but I don't quite understand the problem you're seeing. By "adding new contexts might break things" do you mean that if code is changed by a developer adding a Context it could conceivably change the behavior in a not immediately obvious way? (if that's not what you mean can you rephrase?)

Because that is certainly true but that's just true of Context in general I also think that comes with the territory when we're talking about composition. I don't think it's any worse than the kind of behavior that could be caused by throwing promises.

Importantly, you don't need Context often when you have hoist and the situations where you're consuming non-key values from Context are unlikely to be the ones where you're using hoist.

then i see some potential challenges around effects

Yep, that's definitely an area that I'm not sure what the best way to handle it would be. Right now I'm leaning towards making the "hoisted store" persist until the Provider unmounts which avoids any weird pitfalls like the ones you mentioned but that makes things leaky. My best idea for handling the leak would be adding a hook like useShouldUnmount.

function useSelectedColorState () {
  const id = useContext (ProductIdContext)
  useShouldUnmount (() => {
    return true
  }, [ id ])

  const defaultColor = useDefaultColor ()
  return useState (defaultColor)
}

and do we NEED this to handle effects and arbitrary hooks at all?

Yep, we already have Recoil/Jotai that work like your keyed store. Both of those libraries support effects, derived state, reducers, error handling and Suspense out of necessity. That's part of what brought me to trying to integrate this idea with React properly, if you're going to have to support all of those things then you might as well support hooks fully.

maybe having a keyed updatable store would be enough?

Even Recoil/Jotai weren't enough when I tried using them. The problem was they're always at a fixed-scope (usually global) which covers a lot of common use cases but they are not complete. You need to be able to have "stores" at different scopes that can interop.

what usecases is this missing?

The RFC's Full Example is probably the smallest example I can make that demonstrates a true use case for hoist. I'll try to make that one more digestible maybe with a visual component, it's kind of obnoxious for a so-called example.

@lubieowoce
Copy link

lubieowoce commented Feb 19, 2023

By "adding new contexts might break things" do you mean that if code is changed by a developer adding a Context it could conceivably change the behavior in a not immediately obvious way?

Right, i guess you've got your first instance of a user getting confused by the semantics :D The explanation is a bit long, so please bear with me here. The scenario I was imagining is something like this:

Imagine a UI for a Post (maybe like Twitter) that's collapsible/exapandable. You've got

  1. <PostBody> -- shows the post content. if collapsed, it's a preview, if expanded, it's the full post. clicking on it expands it.
  2. <PostButtons> -- some ui to interact with the post, like liking. also has an "Expand/Collapse" button, to make it interesting.

Of course, you want both of those to share the expanded/collapsed state, so you do something like this:

const PostIdCtx = createContext(null);

// just a simple wrapper around some state to get `{ id, isExpanded, expand(), collapse() }.
// could be a useReducer if that's your jam
const usePostState = () => {
  // this means that, if we hoist, we'll get moved up to the nearest `PostIdCtx.Provider`, right?
  const id = useContext(PostIdCtx);
  
  const [isExpanded, setIsExpanded] = useState(false);
  
  const expand = () => {
    setIsExpanded(true);
  }
  
  const collapse = () => {
    setIsExpanded(false);
  }
  
  return { id, isExpanded, expand, collapse }
}


 const useHoistedPostState = hoist(() => {
   return usePostState();
 });
 
 
 // Our tree looks like this:
 <>
   <PostIdCtx.Provider value={1}> {/* conceptually, useHoistedPostState lives here */}
     {/* these two call useHoistedPostState, so they'll share the same state */}
     <PostBody />    
     <PostButtons />
    </PostIdCtx.Provider>
 </>

All is well in the world. State is shared correctly, because it gets hoisted up.

But then, someone decides we need to track analytics for expand/collapse separately for the body and the buttons to know which one the users prefer. And we clearly like contexts, so we cook up something like this:

   <PostIdCtx.Provider value={1}> {/* conceptually, useHoistedPostState lives here */}
     {/* these two call useHoistedPostState, so they'll share the same value */}
+    {/* ...or will they? */}   
+    <AnalyticsContext source="post-body">
       <PostBody />
+    </AnalyticsContext>
+    <AnalyticsContext source="post-buttons">
       <PostButtons />
+    </AnalyticsContext>
   </PostIdCtx.Provider>
const usePostState = () => {
  // this means that, if we hoist, we'll get moved up to the nearest `PostIdCtx.Provider`, right?
  const id = useContext(PostIdCtx);
+ // but wait, AnalyticsContext is lower down in the tree... so hoisting will break now? 
+ const { source, track } = useAnalyticsContext();

  const [isExpanded, setIsExpanded] = useState(false)
  const expand = () => {
+   track('post-expand', source);
    setIsExpanded(true);
  }
  
  const collapse = () => {
+   track('post-collapse', source);
    setIsExpanded(false);
  }
  
  return { id, isExpanded, expand, collapse }
}

(apologies for the syntax highlighting, i can't seem to find a way to both show the diff and the syntax)

Now, as far i understand, this'd mean that the state will no longer be shared between PostBody and PostButtons -- if we're hoisting the hook up to the innermost context referenced in it, surely that means we get two separate copies now, because AnalyticsContext is the innermost one, right? So now our "expand/collapse" in PostButtons is broken, because PostBody will no longer "see" those updates.

The part where i was kinda wrong is: if instead of using hoist() we just had a "vanilla context" implementation, i.e. a <PostStateProvider> sitting above the two components that just puts usePostState() into a context, this wouldn't work either! though for a different reason -- <PostStateProvider> wouldn't be able to see those AnalyticsContexts because it sits above them in the tree. so you'd have to add a wrapper hook (perhaps usePostStateWithAnalytics) that adds the analytics stuff from "lower down in the tree", where you can access those. And this same workaround could be applied in the hoist()ed version, so fair enough.

HOWEVER i think this'd be much harder too debug with hoist() -- in the "vanilla context" version, you should be able to tell where you went wrong relatively easily -- "oh the analytics are wrong because i forgot that those contexts sit lower down, my bad". Whereas, with hoist()'s semantics as i understand them, you'd get the correct analytics, BUT the states would suddenly no longer be in sync -- each of the components would get its own useState, because now AnalyticsContext is the "innermost" context, so that's as far as hoist() gets lifted. Do i have that right?

@rcharmeyer
Copy link
Author

rcharmeyer commented Feb 19, 2023

Thanks for taking the time to come up with this example!

Yes, you're completely correct. This API isn't a breaking change but it does retcon how we think about Context and what appropriate uses for it are. It's definitely one of the downsides and I think you make a good point about how the symptom isn't aligned with the cause.

Since the issue is really about expectations and troubleshooting maybe it's just something where we need an additional API that could allow users to make assertions about how they expect hoist to work. Maybe something as simple as useHoistingContext (context) as a copy of useContext which just logs a warning or throws an error if it's not the level hoisted to.

Additionally, we may want a couple other ones like useNoHoist() that allow users to make similar assertions about how the hooks they write are being used. Unlike a lot of other use cases for verifying hooks aren't misused, these would probably need to be in React.

I do think this is well worth it even if this issue were left unaddressed.

Ultimately, AnalyticsContext is statically used by the hoisted hook (even if it may be a transitive dependency). If AnalyticsContext is one of those contexts that is supposed to be used in many different ways that's at least something the person using hoist can be aware of and they have the ability to workaround.

EDIT:

So I forgot to mention it before but there is definitely a "right" way to write these hoisted hooks and it's to keep them minimal and compose them. By breaking them up into smaller pieces they will hoist correctly.

const usePostExpandedState = hoist (() => {
  const id = useContext(PostIdCtx)
  return useState(false)
})

const usePostState = hoist (() => {
  const id = useContext (PostIdCtx)
  const [isExpanded, setIsExpanded] = usePostExpandedState()
  const { source, track } = useAnalyticsContext()
  
  const expand = () => {
    track('post-expand', source)
    setIsExpanded(true)
  }
  
  const collapse = () => {
    track('post-collapse', source)
    setIsExpanded(false)
  }
  
  return { id, isExpanded, expand, collapse }
}

@rcharmeyer rcharmeyer changed the title RFC: Hoist API RFC: Hoisted Stores Apr 5, 2023
@rcharmeyer
Copy link
Author

For feature readers, everything above this comment was for an older version of the RFC where there was no createScope. Instead the scope was determined by what Contexts were used. This was based on a false assumption by me that useContext expected a constant argument.

@bowheart
Copy link

Very interesting idea and discussion. I'm just coming in and it's a little hard to understand the use cases. I get the part about new Providers unmounting the tree, but I can't think of a situation where I'd ever need a fix for that - including with crazy code-splitting scenarios. But even hypothetically:

Just letting the tree unmount would usually be fine. In the rare cases where it isn't, I'd rather render a temporarily-empty provider and un-code-split some code than make my team learn a brand new API that we'll likely never use again (if use cases are as rare as they seem).

Sorry, don't mean to be a downer. I'm genuinely curious if there are real use cases I'm missing and a little sad to see you stopped maintaining this RFC for lack of interest. Though - and maybe I'm speaking for some of these disinterested people - the examples I see in the RFC and this discussion don't seem to merit a native React API. A codesandbox illustrating a real existing problem would go a long way.

Specifically this:

hoist is better because of use cases involving lazy loading, lists and errors/suspense

I'd love to see examples demonstrating why existing tools are insufficient.

Even Recoil/Jotai weren't enough when I tried using them. The problem was they're always at a fixed-scope (usually global) which covers a lot of common use cases but they are not complete. You need to be able to have "stores" at different scopes that can interop.

This sounds similar to Jotai's molecules. Those seem powerful enough to me to tackle any examples I see here. Let me know if I'm missing something.

I'm also very interested in seeing your atomic library. Welcome to the club! I've also written one of those. Ignore the haters. Toxicity in the React state management community is seriously crushing innovation. Don't let it!

This discussion in our repo about atom scoping may be of interest.

@rcharmeyer
Copy link
Author

@bowheart thanks for your comment! I'll try and answer your questions. FYI there is an (old) sandbox in the post and the "Product Page" example is the real-world use case I ran in to.

I know that the RFC is kind of limited with examples, before I got feedback that there were too many examples. Ultimately a single Markdown document is a pretty big limitation and I'm not the strongest technical writer to begin with!

This whole 'hoisted hooks' model "just works" to a degree that existing solutions don't. There isn't anything that can be expressed using state management libraries that couldn't be expressed with this API but that doesn't go the other way. That expressiveness means it's deterministic. Think about when you write functional React components without props or context. You can just write a component and know that it works. These 'hoisted hooks' work like that!

Even the 'providing' part is predictable and independent, there is an objectively right answer to where you place them and it has nothing to do with what you're hoisting. Providing in this world makes sense on it's own and that's a huge difference compared to how we often use context, where it's kind of part of expressing the behavior.

Molecules

jotai-molecules is for sure similar but it has a fatal flaw: it uses the value as the scope. If you ever have a value collision, it's not going to work in a predictable manner. Like in the Counter example for jotai-molecules you have to have different initialValues for the Counters to have them track different state, if they both start at 0 then they share their state. Even controlling the Provider, the behavior is non-deterministic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants