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.

Sign up for GitHub

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

Jump to bottom

Removed an unnecessary wrapper object from state #12383

Merged
merged 4 commits into from Mar 15, 2018
Merged

Removed an unnecessary wrapper object from state #12383

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e302ec6
Removed an unecessary wrapper object from state
bvaughn Mar 15, 2018
b4622af
Moved unsubscribe from state to class field
bvaughn Mar 15, 2018
2c86ad6
Commits tweak
bvaughn Mar 15, 2018
f6b6a50
Explicitly null out unsubscribe
bvaughn Mar 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 9 additions & 18 deletions packages/create-subscription/src/createSubscription.js
View file
Open in desktop
Expand Up @@ -51,32 +51,25 @@ export function createSubscription<Property, Value>(
};
type State = {
source: Property,
unsubscribeContainer: {
unsubscribe: Unsubscribe | null,
},
value: Value | void,
};

// Reference: https://gist.github.com/bvaughn/d569177d70b50b58bff69c3c4a5353f3
class Subscription extends React.Component<Props, State> {
state: State = {
source: this.props.source,
unsubscribeContainer: {
unsubscribe: null,
},
value:
this.props.source != null
? getCurrentValue(this.props.source)
: undefined,
};

_unsubscribe: Unsubscribe | null = null;

static getDerivedStateFromProps(nextProps, prevState) {
if (nextProps.source !== prevState.source) {
return {
source: nextProps.source,
unsubscribeContainer: {
unsubscribe: null,
},
value:
nextProps.source != null
? getCurrentValue(nextProps.source)
Expand Down Expand Up @@ -125,18 +118,16 @@ export function createSubscription<Property, Value>(
});
};

// Store subscription for later (in case it's needed to unsubscribe).
// This is safe to do via mutation since:
// 1) It does not impact render.
// 2) This method will only be called during the "commit" phase.
// Store the unsubscribe method for later (in case the subscribable prop changes).
const unsubscribe = subscribe(source, callback);

invariant(
typeof unsubscribe === 'function',
'A subscription must return an unsubscribe function.',
);

this.state.unsubscribeContainer.unsubscribe = unsubscribe;
// It's safe to store unsubscribe on the instance because
// We only read or write that property during the "commit" phase.
this._unsubscribe = unsubscribe;

// External values could change between render and mount,
// In some cases it may be important to handle this case.
Expand All @@ -148,10 +139,10 @@ export function createSubscription<Property, Value>(
}

unsubscribe(state: State) {
const {unsubscribe} = state.unsubscribeContainer;
if (typeof unsubscribe === 'function') {
unsubscribe();
if (typeof this._unsubscribe === 'function') {
this._unsubscribe();
}
this._unsubscribe = null;
}
}

Expand Down