Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@ version: 2
jobs:
build:
docker:
- image: circleci/node:12.14
- image: cimg/node:14.21

steps:
- run:
name: Node version
command: node -v
- checkout
- restore_cache:
keys:
Expand Down
12 changes: 8 additions & 4 deletions src/react/connectAs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,21 @@ export function connectAs<
stores: {
[K in keyof Stores]: ReturnType<Stores[K]['getCurrentSnapshot']>
}
subscriptions: Subscription[]
}

return class extends React.Component<Diff<Props, Stores>, State> {
static displayName = `withStore(${getDisplayName(Component)})`
subscriptions: Subscription[] = []
state = {
stores: mapValues(
stores,
_ =>
_.getCurrentSnapshot() as ReturnType<typeof _['getCurrentSnapshot']>
),
subscriptions: keys(stores).map(k =>
)
}

componentDidMount(): void {
this.subscriptions = keys(stores).map(k =>
stores[k].onAll().subscribe(({ previousValue, value }) => {
if (equals(previousValue, value)) {
return false
Expand All @@ -44,7 +47,8 @@ export function connectAs<
}

componentWillUnmount() {
this.state.subscriptions.forEach(_ => _.unsubscribe())
this.subscriptions.forEach(_ => _.unsubscribe())
this.subscriptions = []
}

shouldComponentUpdate(props: Diff<Props, Stores>, state: State) {
Expand Down
21 changes: 14 additions & 7 deletions src/react/createConnectedStore.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export function createConnectedStore<State extends object>(
ContainerProps<State>,
ContainerState
> {
subscription: Subscription
subscription: Subscription | null = null
storeDefinition: StoreDefinition<State>
constructor(props: ContainerProps<State>) {
super(props)
Expand All @@ -45,23 +45,30 @@ export function createConnectedStore<State extends object>(
fx(this.storeDefinition)
}

this.subscription = this._createSubscription()
this.state = {
storeSnapshot: this.storeDefinition.getCurrentSnapshot()
}
}

this.subscription = this.storeDefinition.onAll().subscribe(() =>
_createSubscription = () =>
this.storeDefinition.onAll().subscribe(() =>
this.setState({
storeSnapshot: this.storeDefinition.getCurrentSnapshot()
})
)

componentDidMount(): void {
if (this.subscription == null) {
this.subscription = this._createSubscription()
}
}

componentWillUnmount() {
this.subscription.unsubscribe()
// Let the state get GC'd.
// TODO: Find a more elegant way to do this.
;(this.storeDefinition as any).storeSnapshot = null
;(this as any).storeDefinition = null
if (this.subscription != null) {
this.subscription.unsubscribe()
this.subscription = null
Copy link
Owner

Choose a reason for hiding this comment

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

Can we keep the GC lines? These were pretty important for long-running apps with complex state, IIRC.

Suggested change
this.subscription = null
this.subscription = null
// Let the state get GC'd.
// TODO: Find a more elegant way to do this.
;(this.storeDefinition as any).storeSnapshot = null
;(this as any).storeDefinition = null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

componentDidMount above requires access to the storeDefinition to re-establish the subscription. Not sure there's a way around keeping the reference here?

}
}

render() {
Expand Down
50 changes: 25 additions & 25 deletions src/react/createConnectedStoreAs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,16 @@ export function createConnectedStoreAs<
let Context = React.createContext({ __MISSING_PROVIDER__: true } as any)

type ContainerState = {
storeDefinitions: { [K in keyof States]: StoreDefinition<States[K]> | null }
storeSnapshots: { [K in keyof States]: StoreSnapshot<States[K]> | null }
subscriptions: { [K in keyof States]: Subscription }
}

class Container extends React.Component<
ContainerPropsAs<States>,
ContainerState
> {
storeDefinitions: { [K in keyof States]: StoreDefinition<States[K]> }
subscriptions: { [K in keyof States]: Subscription } | null

constructor(props: ContainerPropsAs<States>) {
super(props)

Expand All @@ -65,37 +66,36 @@ export function createConnectedStoreAs<
fx(stores as any) // TODO
}

this.storeDefinitions = stores as any
this.subscriptions = this._createSubscriptions()
this.state = {
storeDefinitions: stores as any, // TODO
// TODO
storeSnapshots: mapValues(stores, _ => _.getCurrentSnapshot()) as any,
subscriptions: mapValues(stores, (_, k) =>
_.onAll().subscribe(() =>
this.setState(state => ({
storeSnapshots: Object.assign({}, state.storeSnapshots, {
[k]: _.getCurrentSnapshot()
})
}))
)
storeSnapshots: mapValues(stores, _ => _.getCurrentSnapshot()) as any
}
}

_createSubscriptions = () =>
mapValues(this.storeDefinitions, (_, k) =>
_.onAll().subscribe(() =>
this.setState(state => ({
storeSnapshots: Object.assign({}, state.storeSnapshots, {
[k]: _.getCurrentSnapshot()
})
}))
)
) as any
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: Do you need this as any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can double check, I feel like I had basically just moved these lines but could see if I can fix at least some of them.


componentDidMount(): void {
if (this.subscriptions == null) {
this.subscriptions = this._createSubscriptions()
}
}

componentWillUnmount() {
mapValues(this.state.subscriptions, _ => _.unsubscribe())
// Let the state get GC'd.
// TODO: Find a more elegant way to do this.
if (this.state.storeSnapshots) {
if (this.subscriptions != null) {
mapValues(this.subscriptions, _ => _.unsubscribe())
this.subscriptions = null
}
mapValues(this.state.storeSnapshots, _ => ((_ as any).state = null))
mapValues(
this.state.storeSnapshots,
_ => ((_ as any).storeDefinition = null)
Copy link
Owner

Choose a reason for hiding this comment

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

Same here -- is there a way we could keep this GC-related code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above, we need to re-establish the subscriptions.

)
mapValues(
this.state.storeDefinitions,
_ => ((_ as any).storeSnapshot = null)
)
}

render() {
Expand Down