Skip to content

Conversation

@d3dc
Copy link
Contributor

@d3dc d3dc commented May 23, 2018

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:

  • a store is ready for use after createStore().

  • <Provider /> now only needs to subscribe to state changes and uses ordinary React setState.


Examples

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

@drcmda
Copy link
Owner

drcmda commented May 23, 2018

@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
Copy link
Contributor Author

d3dc commented May 24, 2018

@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
Copy link
Contributor Author

d3dc commented May 24, 2018

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
Copy link
Contributor Author

d3dc commented May 24, 2018

@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?

d3dc added 7 commits May 23, 2018 21:01
External stores have a `setState` function
External stores have `state.setState` by default.
External stores have a `wrapActions` function to provide middleware on this side (should this use subscriptions?)

The first middleware is always `store.setState`
Remove public setState.
Move state creation to its own function.
Overwritten actions are rebound to only affect this Provider
@codecov-io
Copy link

codecov-io commented May 24, 2018

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%> (ø) ⬆️

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
Copy link
Owner

drcmda commented May 24, 2018

@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
Copy link
Contributor Author

d3dc commented May 25, 2018

@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
Copy link
Owner

drcmda commented May 25, 2018

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
Copy link
Contributor Author

d3dc commented May 25, 2018

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.

Details I added the extra state because of how my app previously did initial state for the repeated providers: ```js ```

it could now do:

<Provider store={FormSectionStore} drawersOpen={false} ... >

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):

<Provider store={SessionStore} initialState={{ drawersOpen: true }} ... >

@d3dc
Copy link
Contributor Author

d3dc commented Jun 4, 2018

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.

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.

External Stores stop being external as soon as you provide them

3 participants