drcmda / react-contextual

🚀 react-contextual is a small (less than 1KB) helper around React 16s new context api
MIT License
642 stars 22 forks source link

Standalone External Stores #25

Open d3dc opened 6 years ago

d3dc commented 6 years ago

While looking for a way to fix #24, I wondered off, so I'm open to as big of suggestions as anyone wants to give.

Currently, external stores and Providers are a little confusing. The constructor for a Provider actually finishes constructing the "external" store. I understand this to be a two-way binding more now - the Provider's state is made external to React.

This could be improved upon, making the relationship explicit and subscribe-able. But that's not how I was originally using them or what I've done here...

This PR moves all the construction of external stores to createStore and introduces a new step in construction: wrapStateUpdateFunctions(state, store, callback).

API Changes:


Examples

Two Providers, one store (naive coder?): https://codesandbox.io/s/jp46nlpk8y?view=preview

drcmda commented 6 years ago

@d3dc This is pretty cool! Yeah it goes way further than i expected. But makes sense to keep the logic inside the store container. As for middleware, do we need it? There's a declarative middleware sort-of construct: https://codesandbox.io/embed/mjv84k1kn9 But maybe for logging, re-winding state, etc. - could be useful.

d3dc commented 6 years ago

@drcmda I already use transformContext in our code base - I don't know why I never thought of some of our side effects as derived state. I'll have to spend more time thinking about it.

I use the word middleware specifically because of the standard redux use cases of logging and debugging. I can see scenario's where wrapStateUpdateFunctions would be use, but perhaps the public interface for it can remain in the air for now.

d3dc commented 6 years ago

Old Examples: Something to do with middleware

export const AuthStore = createStore({
  user: null,
  setUser: user => ({ user })
})

...

// Logging
AuthStore.subscribeActions((payload, action) => {
  console.log(`received [${action}]`, payload)
})
...

// Side effect
AuthStore.subscribeActions((payload, action) => {
  switch (action) {
    case 'setUser': 
      if (payload.user == null) {
        DataStore.state.setData(null)
      }
      return
  }
})
d3dc commented 6 years ago

@drcmda I went ahead and set this up to not make too much public API. Providers now have a subscription to their store and will re-render according to ordinary setState rules.

If you pass an action as a prop to a Provider, what should happen? Should the Provider provide an action that only updates its internal state? Or should the Provider wrap that action to affect the external store?

codecov-io commented 6 years ago

Codecov Report

Merging #25 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #25   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines          98    101    +3     
=====================================
+ Hits           98    101    +3
Impacted Files Coverage Δ
src/store.js 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 26ded70...e052e31. Read the comment docs.

drcmda commented 6 years ago

@d3dc hard to say, can you make a small example?

Btw, do you need more access? I'd be totally open to share authorship if good ideas find their way into the lib.

d3dc commented 6 years ago

@drcmda I'm not sure what else I should be contributing. I detail a lot since I feel like I'm stepping on the toes of a "less than 1kb" opinionated library.

As far as Provider examples go:

a simple Provider has no passed store:

<Provider toggled={false} toggle={() => state => ({ toggled: !state.toggled })} >
  {children}
</Provider>

a Provider with a store then should keep that behavior

const GlobalToggleStore = createStore({
  toggled: false,
  toggle: () => state => ({ toggled: !state.toggled })
})

...

<Provider store={GlobalToggleStore} toggled={false} toggle={() => ({ toggled: false })} >
  {children}
</Provider>

It seems wrong to be able to "add functionality" to GlobalToggleStore with props on a Provider; so additional actions will only effect the Provider.

At this point, the Provider is subscribing to a store and also providing its internal state. In the case where no store is passed, it still constructs a store just to subscribe to it. Provider could be even simpler and just provide its internal state with any action props wrapped in simple React setState.

This removes the clumsy binding and makes Provider more of a direct store or "a component that provides its own state and can subscribe to external state"

drcmda commented 6 years ago

I think it should disregard other props when a store is given. I mean it could override of course, but that again would mutate the store. Anyway, generally i think it's best to make it as simple as possible, better removing a few options rather than having something that can do all but it's getting hard to guess at what it's actually doing.

d3dc commented 6 years ago

Your concern about overriding is why I made the change to provider. Any actions you pass to a provider will always only mutate the provider. In the same vein, any values you pass to a provider will always only be available to it's children.

Passing a store only subscribes to it. If it's state has actions, they are just passed along - any store actions will always only mutate the store (which of course will notify the Provider and overwrite its initial values).


I do think it was perfectly clear with: it uses the store or constructs a store with its props and can change it back.

I added the extra state because of how my app previously did initial state for the repeated providers: ```js ``` it could now do: ```js ``` So the one section has a different initial state, but all interaction with the store then sync it up with the rest of the app. --- I can already see that overridden actions get destroyed pretty quickly. It may be confusing as these are initial values and nothing indicates it - maybe this is just a prop to Provider (but this is additional api): ```js ```
d3dc commented 6 years ago

I reverted my change and also made sure not to change how subscribers would notified. I think this is mergeable if the coverage changes are fine.