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

Port Subscription closure implementation from 8.x to 7.x (#1807) #1809

Merged
merged 1 commit into from Sep 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions src/components/Provider.js
@@ -1,12 +1,12 @@
import React, { useMemo } from 'react'
import PropTypes from 'prop-types'
import { ReactReduxContext } from './Context'
import Subscription from '../utils/Subscription'
import { createSubscription } from '../utils/Subscription'
import { useIsomorphicLayoutEffect } from '../utils/useIsomorphicLayoutEffect'

function Provider({ store, context, children }) {
const contextValue = useMemo(() => {
const subscription = new Subscription(store)
const subscription = createSubscription(store)
subscription.onStateChange = subscription.notifyNestedSubs
return {
store,
Expand Down
4 changes: 2 additions & 2 deletions src/components/connectAdvanced.js
@@ -1,7 +1,7 @@
import hoistStatics from 'hoist-non-react-statics'
import React, { useContext, useMemo, useRef, useReducer } from 'react'
import { isValidElementType, isContextConsumer } from 'react-is'
import Subscription from '../utils/Subscription'
import { createSubscription } from '../utils/Subscription'
import { useIsomorphicLayoutEffect } from '../utils/useIsomorphicLayoutEffect'

import { ReactReduxContext } from './Context'
Expand Down Expand Up @@ -334,7 +334,7 @@ export default function connectAdvanced(

// This Subscription's source should match where store came from: props vs. context. A component
// connected to the store via props shouldn't use subscription from context, or vice versa.
const subscription = new Subscription(
const subscription = createSubscription(
store,
didStoreComeFromProps ? null : contextValue.subscription
)
Expand Down
4 changes: 2 additions & 2 deletions src/hooks/useSelector.js
@@ -1,6 +1,6 @@
import { useReducer, useRef, useMemo, useContext, useDebugValue } from 'react'
import { useReduxContext as useDefaultReduxContext } from './useReduxContext'
import Subscription from '../utils/Subscription'
import { createSubscription } from '../utils/Subscription'
import { useIsomorphicLayoutEffect } from '../utils/useIsomorphicLayoutEffect'
import { ReactReduxContext } from '../components/Context'

Expand All @@ -14,7 +14,7 @@ function useSelectorWithStoreAndSubscription(
) {
const [, forceRender] = useReducer((s) => s + 1, 0)

const subscription = useMemo(() => new Subscription(store, contextSub), [
const subscription = useMemo(() => createSubscription(store, contextSub), [
store,
contextSub,
])
Expand Down
73 changes: 41 additions & 32 deletions src/utils/Subscription.js
Expand Up @@ -4,8 +4,6 @@ import { getBatch } from './batch'
// well as nesting subscriptions of descendant components, so that we can ensure the
// ancestor components re-render before descendants

const nullListeners = { notify() {} }

function createListenerCollection() {
const batch = getBatch()
let first = null
Expand Down Expand Up @@ -71,51 +69,62 @@ function createListenerCollection() {
}
}

export default class Subscription {
constructor(store, parentSub) {
this.store = store
this.parentSub = parentSub
this.unsubscribe = null
this.listeners = nullListeners
const nullListeners = {
notify() {},
get: () => [],
}

this.handleChangeWrapper = this.handleChangeWrapper.bind(this)
}
export function createSubscription(store, parentSub) {
let unsubscribe
let listeners = nullListeners

addNestedSub(listener) {
this.trySubscribe()
return this.listeners.subscribe(listener)
function addNestedSub(listener) {
trySubscribe()
return listeners.subscribe(listener)
}

notifyNestedSubs() {
this.listeners.notify()
function notifyNestedSubs() {
listeners.notify()
}

handleChangeWrapper() {
if (this.onStateChange) {
this.onStateChange()
function handleChangeWrapper() {
if (subscription.onStateChange) {
subscription.onStateChange()
}
}

isSubscribed() {
return Boolean(this.unsubscribe)
function isSubscribed() {
return Boolean(unsubscribe)
}

trySubscribe() {
if (!this.unsubscribe) {
this.unsubscribe = this.parentSub
? this.parentSub.addNestedSub(this.handleChangeWrapper)
: this.store.subscribe(this.handleChangeWrapper)
function trySubscribe() {
if (!unsubscribe) {
unsubscribe = parentSub
? parentSub.addNestedSub(handleChangeWrapper)
: store.subscribe(handleChangeWrapper)

this.listeners = createListenerCollection()
listeners = createListenerCollection()
}
}

tryUnsubscribe() {
if (this.unsubscribe) {
this.unsubscribe()
this.unsubscribe = null
this.listeners.clear()
this.listeners = nullListeners
function tryUnsubscribe() {
if (unsubscribe) {
unsubscribe()
unsubscribe = undefined
listeners.clear()
listeners = nullListeners
}
}

const subscription = {
addNestedSub,
notifyNestedSubs,
handleChangeWrapper,
isSubscribed,
trySubscribe,
tryUnsubscribe,
getListeners: () => listeners,
}

return subscription
}
8 changes: 4 additions & 4 deletions test/hooks/useSelector.spec.js
Expand Up @@ -101,11 +101,11 @@ describe('React', () => {
</ProviderMock>
)

expect(rootSubscription.listeners.get().length).toBe(1)
expect(rootSubscription.getListeners().get().length).toBe(1)

store.dispatch({ type: '' })

expect(rootSubscription.listeners.get().length).toBe(2)
expect(rootSubscription.getListeners().get().length).toBe(2)
})

it('unsubscribes when the component is unmounted', () => {
Expand All @@ -129,11 +129,11 @@ describe('React', () => {
</ProviderMock>
)

expect(rootSubscription.listeners.get().length).toBe(2)
expect(rootSubscription.getListeners().get().length).toBe(2)

store.dispatch({ type: '' })

expect(rootSubscription.listeners.get().length).toBe(1)
expect(rootSubscription.getListeners().get().length).toBe(1)
})

it('notices store updates between render and store subscription effect', () => {
Expand Down
6 changes: 3 additions & 3 deletions test/utils/Subscription.spec.js
@@ -1,4 +1,4 @@
import Subscription from '../../src/utils/Subscription'
import { createSubscription } from '../../src/utils/Subscription'

describe('Subscription', () => {
let notifications
Expand All @@ -9,13 +9,13 @@ describe('Subscription', () => {
notifications = []
store = { subscribe: () => jest.fn() }

parent = new Subscription(store)
parent = createSubscription(store)
parent.onStateChange = () => {}
parent.trySubscribe()
})

function subscribeChild(name) {
const child = new Subscription(store, parent)
const child = createSubscription(store, parent)
child.onStateChange = () => notifications.push(name)
child.trySubscribe()
return child
Expand Down