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

Add ability to take in middleware #18

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

pksjce
Copy link

@pksjce pksjce commented Apr 29, 2018

#17
It looks as follows when being consumed.

import React from 'react';
import createState from 'react-copy-write';

const logger = getState => next => fn => {
  const state = getState();
  console.log('prev state', state);
  next(fn);
  console.log('next state', getState());
}

const crashReporter = getState => next => fn => {
  try {
    next(fn)
  } catch (err) {
    console.error('Caught an exception!', err)

    throw err
  }
}

const { Provider, Consumer, createMutator } = createState({
  count: 0
}, [crashReporter, logger])

const increment = createMutator(draft => draft.count = draft.count + 1);
const decrement = createMutator(draft => draft.count = draft.count - 1);


function Counter() {
  return (
    <Consumer>
      {state => (
        <div>
          <button onClick={decrement}>-</button>
          <span>{state.count}</span>
          <button onClick={increment}>+</button>
        </div>
      )}
    </Consumer>
  );
}

export default () => <Provider>
  <Counter />
</Provider>

What do you think? Need some TLC for typing and tests.

@tsaiDavid
Copy link

I’d love to help on this!

@pksjce
Copy link
Author

pksjce commented May 8, 2018

I'm waiting for @aweary to review it! I feel this is good step to help testing and have some dev tools for this library.

@aweary
Copy link
Owner

aweary commented May 8, 2018

I'm a little busy at the moment, but I'll try to review soon. At a high-level I think the middleware implementation needs to be more simple. The getState API is probably more complex than I'd want. Middleware is somewhat less valuable with react-copy-write compared to Redux, since there's no concept of an action.

If we just pass prevState and nextState to the middleware it would simplify this greatly. I understand the use case of wrapping the update in a try/catch block, but ideally I think I'd rather promote using an error boundary (and ensuring errors thrown in mutators are caught by error boundaries)

Are there any other good use cases for passing fn to middleware?

@pksjce
Copy link
Author

pksjce commented May 9, 2018

If we just pass prevState and nextState to the middleware it would simplify this greatly.

Yeah, this was my first approach.

I'd rather promote using an error boundary

An error boundary at the top level of the app could do the same job. (ie sending the event to sentry or other service)

Are there any other good use cases for passing fn to middleware?

Not that I can think of. I mainly want some middleware to get some kind of dev tooling happen.

You would accept a simpler middleware api then?

@kiliancs
Copy link

IMO Immer integration should be optional and added through middleware. react-copy-write is really thin and that's great. Immer is very opinionated and that's great, but also a good case for making it optional.

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.

8 participants