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

Chainable methods on subscriptions #155

Merged
merged 2 commits into from
Jun 24, 2020
Merged

Chainable methods on subscriptions #155

merged 2 commits into from
Jun 24, 2020

Conversation

jnicklas
Copy link
Collaborator

This adds a free function called subscribe which returns a subscription from a subscribable. It also adds chainable methods via ChainableSubscription which makes it possible to call map, filter and match on subscriptions.

Finally it deprecates Subscribable.from and chaining off of a subscribable.

Closes #154

This adds a free function called `subscribe` which returns a subscription from a subscribable. It also adds chainable methods via `ChainableSubscription` which makes it possible to call `map`, `filter` and `match` on subscriptions.

Finally it deprecates `Subscribable.from` and chaining off of a subscribable.
@jnicklas jnicklas requested a review from cowboyd June 22, 2020 17:48
@changeset-bot
Copy link

changeset-bot bot commented Jun 22, 2020

🦋 Changeset is good to go

Latest commit: 784a0bb

We got this.

This PR includes changesets to release 2 packages
Name Type
@effection/subscription Minor
@effection/events Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jun 22, 2020

The preview packages of this pull request have been published.
Click on the following packages for instructions on how to install them:

@effection/fetch

Install using the following command:

$ npm install @effection/fetch@subscription-chainable

Or update your package.json file:

{
  "@effection/fetch": "subscription-chainable"
}

@effection/node

Install using the following command:

$ npm install @effection/node@subscription-chainable

Or update your package.json file:

{
  "@effection/node": "subscription-chainable"
}

@effection/subscription

Install using the following command:

$ npm install @effection/subscription@subscription-chainable

Or update your package.json file:

{
  "@effection/subscription": "subscription-chainable"
}

Generated by 🚫 dangerJS against 784a0bb

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be that the biggest difference here, is whether you're transforming the subscription itself, or the operation that produces the subscription. Are these semi-orthogonal use cases? With the chainable subscription, you need to go through a two step process:

  1. produce the chainable subscription from the subscribe operation.
  2. chain off the chainable subscription:
let subscription: ChainableSubscription<number, void> = yield subscribe(thing);
let first: number = yield subscription.filter(predicate).expect();

vs chaining the operation that produces the subscription in which you:

  1. create the operation chain.
  2. produce the subscription yielded by the chain
let first: number = yield Subscribable.from(thing).filter(predicate).expect();

There are a couple tradeoffs for going either way, but from what I can see, there are a couple of advantages to having the operation be transformable rather that the value be transformable. Specifically, if the operation is transformable, then it can be defined outside of any particular effection context. Also, (and this is because of TypeScript's flaws around Generator typing) the chaining happens all before hand, meaning that you need less annotations to propagate type information across the steps of the chain. In other words, ChainableSubscription<number, void> which is a keyboard full, is not necessary.

This is all to say that maybe we should have both and not deprecate Subscribable.from() for the moment? What is the exact use-case enabled by transforming the value, and not the operation that produces the value? Perhaps it would make sense to have them co-exist for awhile and then we can either unify or maintain both based on more data.

Let's keep it around for a while and see how subscriptions pan out.
@jnicklas
Copy link
Collaborator Author

That makes sense to me. I pushed a commit to undo the deprecation. Let's keep both for a while and see how it pans out.

@jnicklas jnicklas merged commit 6224a2d into master Jun 24, 2020
@jnicklas jnicklas deleted the subscription-chainable branch June 24, 2020 15:05
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.

Subscribables vs subscriptions
2 participants