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

create-subscription #12325

Merged
merged 54 commits into from Mar 13, 2018
Merged

Conversation

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Mar 4, 2018

Async-safe subscriptions are hard to get right.

create-subscription provides an simple, async-safe interface to manage a subscription.

View the formatted README to learn more...

@bvaughn bvaughn force-pushed the bvaughn:create-component-with-subscriptions branch from 47af8a3 to 11e741c Mar 5, 2018
@bvaughn bvaughn force-pushed the bvaughn:create-component-with-subscriptions branch from 11e741c to d5d8bf6 Mar 5, 2018
@pull-bot
Copy link

@pull-bot pull-bot commented Mar 5, 2018

Details of bundled changes.

Comparing: ad9544f...6f740d9

create-subscription

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
create-subscription.development.js n/a n/a 0 B 5.49 KB 0 B 1.93 KB NODE_DEV
create-subscription.production.min.js n/a n/a 0 B 2.59 KB 0 B 1.22 KB NODE_PROD

Generated by 🚫 dangerJS

@bvaughn bvaughn force-pushed the bvaughn:create-component-with-subscriptions branch from f8ee765 to 4304b55 Mar 5, 2018
@@ -254,6 +254,16 @@ const bundles = [
global: 'SimpleCacheProvider',
externals: ['react'],
},

/******* Simple Cache Provider (experimental) *******/

This comment has been minimized.

@flarnie

flarnie Mar 5, 2018
Contributor

nit - update comment

This comment has been minimized.

@bvaughn

bvaughn Mar 5, 2018
Author Contributor

D'oh 😁

@flarnie
Copy link
Contributor

@flarnie flarnie commented Mar 5, 2018

I can confirm that this, or something like it, will be essential for future React use with subscriptions. Will do a more in-depth review of the implementation later, but so far looks good and fits with all our learning from updating Relay.

I also put it on the agenda for team sync. tomorrow.

@acdlite
Copy link
Member

@acdlite acdlite commented Mar 5, 2018

Cool! I like the idea of providing an foolproof way to do subscriptions in async.

Some concerns:

  • We should try to avoid making it too easy to use subscriptions in cases where another pattern is more idiomatic. For example, one of the uses of subscriptions today is as a workaround for context (parent-child communication). For those cases, you shouldn't use subscriptions—you should use the new context API. If you're integrating with an external data source, subscriptions are probably the way to go, but you still want to avoid having multiple components subscribe to the same source, and instead subscribe once and use the new context API to share the value throughout the tree. Maybe there's a way we can integrate this into the new context API directly so people aren't tempted to oversubscribe.
  • I question the necessity of supporting multiple "subscribable types." An alternative is to expose a generic interface and leave it to users to integrate it with their preferred async primitive. I like that you've proven that this can work with DOM events and observables, but if we provide first-class support for some primitives, inevitably people will ask us to add support for their favorite library. And there are so many: RxJS, most, kefir, xstream, callbag... @gaearon and I faced a similar problem with Redux three years ago (wtf how has it been that long), which is why we decided on the middleware design: keep the core API surface area small and easy to maintain, and let others build abstractions on top of it if they desire. [Edit: Oooooops, ignore this second bullet: it looks like this exactly what you've done :D I misunderstood the issue description.]
@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Mar 5, 2018

Thanks for the feedback, @acdlite!

Totally agreed with your second bullet point! I had that same concern based on past experiences with open source libs. 👍

As for your first concern, I'm on the fence about the best way to handle it. On the one hand, I want it to be as possible- but I hear your concern about encouraging people to use subscriptions in lieu of context.

Maybe there's a way we can integrate this into the new context API directly so people aren't tempted to oversubscribe.

I could export another HOC that ties into the context API, if you think it would be worth doing? It wouldn't be much extra code.

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Mar 5, 2018

I've updated the README with an API overview and some basic examples for event dispatchers and RxJS. I'd like to add more examples as I think of them. (Ideas welcome!)

@bvaughn bvaughn mentioned this pull request Mar 5, 2018
2 of 2 tasks complete
@bvaughn bvaughn force-pushed the bvaughn:create-component-with-subscriptions branch from 18c193f to c395051 Mar 5, 2018
@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Mar 5, 2018

I also added documentation and a test for native Promises, although it's a bit clunky. The lack of a way to "unsubscribe" could also result in memory leaks as long as something has a reference to the old Promise, but I think it's still worth documenting how they could be used. (I'm open for feedback here as well of course.)

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Mar 6, 2018

Just an FYI (in case I lose connectivity in the air) - as soon as I'm done writing more tests, I'm going to update this helper to only use the HOC approach for functional components. For class components, I'll use an "ES6 mixin" approach to retain ref compatibility and avoid the overhead of an additional fiber.

@bvaughn bvaughn merged commit 00a0e3c into facebook:master Mar 13, 2018
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
@bvaughn bvaughn deleted the bvaughn:create-component-with-subscriptions branch Mar 13, 2018
@BTMPL
Copy link

@BTMPL BTMPL commented Mar 14, 2018

Since the source branch got deleted might want to edit first post to lead to https://github.com/facebook/react/blob/master/packages/create-subscription/README.md

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Mar 14, 2018

Done 👍

LeonYuAng3NT added a commit to LeonYuAng3NT/react that referenced this pull request Mar 22, 2018
create-subscription provides an simple, async-safe interface to manage a subscription.
This was referenced Mar 30, 2018
rhagigi added a commit to rhagigi/react that referenced this pull request Apr 19, 2018
create-subscription provides an simple, async-safe interface to manage a subscription.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet