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

[FEATURE] Pass single object instead of 3 args into epics #577

Open
Sawtaytoes opened this issue Sep 30, 2018 · 0 comments
Open

[FEATURE] Pass single object instead of 3 args into epics #577

Sawtaytoes opened this issue Sep 30, 2018 · 0 comments

Comments

@Sawtaytoes
Copy link

Do you want to request a feature or report a bug?
Feature

What is the current behavior?

const someEpic = (
  action$,
  state$,
  dependencies,
) => (
  action$
  .pipe(
    // etc
  )
)

What is the expected behavior?

const someEpic = ({
  action$,
  state$,
  ...dependencies,
}) => (
  action$
  .pipe(
    // etc
  )
)

Which versions of redux-observable, and which browser and OS are affected by this issue? Did this work in previous versions of redux-observable?
All 0.x.x and 1.x.x versions.

I think the argument list for an epic needs to change. Because of the way the arguments are setup, I think it can be confusing to new users. For instance, I never thought to pipe off state$ directly until after 9 months of near-daily use. Personally, I also see 3 arguments as too many. At the point you have 3 arguments, it should probably be sending those arguments as named-props instead.

I'd like to see a single object instead. This would make it a lot easier to reach your dependencies. For instance, you might not use action$ or state$ but need some dependency. In the majority case though, you're going to either use action$ or state$ and possibly a dependency. Wouldn't it be easier to select them out of a single object rather than naming all 3 arguments?

I'm sure this can be deprecated so long as you don't use uglify:

const isUsingOldApi = (
  epicCreator,
) => (
  epicCreator.length > 1
  || (
    epicCreator
    .toString()
    .split(' ')
    .join('')
    .includes('(action$')
  )
)

const combineEpics = (
  (
    action$,
    state$,
    dependencies,
  ) => (
    (...epicCreators) => (
      epicCreators
      .map(epicCreator => (
        isUsingOldApi(
          epicCreator,
        )
        ? (
          deprecationMessage(
            "Using old API version. Get with the times.",
            epicCreator(
              action$,
              state$,
              dependencies,
            ),
          )
        )
        : (
          epicCreator({
            ...dependencies,
            action$,
            state$,
          })
        )
      ))
    )
  )
}

Reading this, I just realized I could do the same thing and not bother making it official, but then I'd have epics that don't match anything else out there. Doesn't seem like a best-practice situation to be the odd man out.

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

1 participant