-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
An update on async rendering #596
Changes from 3 commits
544c0bc
e298b47
9ff0f12
0674c34
cdb4b9e
289a2da
99fedea
49464f7
fe6b133
1314117
f005c04
06d5be4
3696388
ac23b1f
5cae7c6
f70c0dd
c1e67be
75a43aa
fb3b91f
60d65ce
f632f22
626ac42
7456327
2909738
5400338
813be17
8de7dc4
c45fb40
858c1a7
98d5a09
442591c
b1ce572
2312173
16eb646
4d16523
9905159
1ca6cfc
7408e07
3c75def
55650fc
97a109d
21fa116
92cf72d
fa34fcf
b3bf0bd
254fc8b
b0c22f7
558d576
7425aed
65b1496
6eae811
a2139de
030980e
e110ac5
ce060eb
e143823
65eca09
a3ea63a
7ced9ce
7cf5b58
9f72403
712f4de
4610392
b824bd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,26 @@ | ||
import {createSubscription} from 'create-subscription'; | ||
|
||
const Subscription = createSubscription({ | ||
getCurrentValue(source) { | ||
// Return the current value of the subscription (source). | ||
getCurrentValue(sourceProp) { | ||
// Return the current value of the subscription (sourceProp). | ||
// highlight-next-line | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO the highlighting in this code sample is distracting and I can't tell why some lines are highlighted and not others. I'd just drop all the highlights in this file especially since it isn't really a before/after and ~all the lines are significant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool. I was on the fence about this to begin with. |
||
return source.value; | ||
return sourceProp.value; | ||
}, | ||
|
||
subscribe(source, callback) { | ||
subscribe(sourceProp, callback) { | ||
function handleSubscriptionChange() { | ||
callback(dataSource.value); | ||
callback(sourceProp.value); | ||
} | ||
|
||
// Subscribe (e.g. add an event listener) to the subscription (source). | ||
// Subscribe (e.g. add an event listener) to the subscription (sourceProp). | ||
// Call callback(newValue) whenever a subscription changes. | ||
// highlight-next-line | ||
source.subscribe(handleSubscriptionChange); | ||
sourceProp.subscribe(handleSubscriptionChange); | ||
|
||
// Return an unsubscribe method. | ||
// highlight-range{1-3} | ||
return function unsubscribe() { | ||
source.unsubscribe(handleSubscriptionChange); | ||
sourceProp.unsubscribe(handleSubscriptionChange); | ||
}; | ||
}, | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's important to point out our migration path isn't "tell every product team to set everything aside and start fixing their components". People commonly assume that we have this kind of power at Facebook and that product teams just go ahead and spend months catching up with our recommendations (and a "small company wouldn't be able to do this"). So I would prefer if we were very clear in our wording that this is not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Gotcha.
I don't think the previous wording, "we can't rewrite our apps", conveyed that. (It didn't really make sense to me.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure—you're a native speaker so I was just trying to give you the context into what I was trying to express.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I wasn't trying to criticize the wording or say it didn't sound native. I just don't think it conveyed the meaning you are wanting to convey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I like it