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

Attempt to make the useSubscription hook simpler by removing reactivity #305

Closed
wants to merge 3 commits into from
Closed
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
71 changes: 41 additions & 30 deletions packages/react-mongo/react-mongo.ts
Original file line number Diff line number Diff line change
@@ -1,62 +1,73 @@
import { Meteor } from 'meteor/meteor'
import { Mongo } from 'meteor/mongo'
import { Tracker } from 'meteor/tracker'
import { useEffect, useMemo, useReducer, useRef, DependencyList } from 'react'
import {
useEffect,
useMemo,
useReducer,
useRef,
useCallback,
DependencyList,
} from 'react'

const fur = (x: number): number => x + 1
const useForceUpdate = () => useReducer(fur, 0)[1]

const useSubscriptionClient = (factory: () => Meteor.SubscriptionHandle | void, deps: DependencyList= []) => {
const forceUpdate = useForceUpdate()
const { current: refs } = useRef<{
handle?: Meteor.SubscriptionHandle,
updateOnReady: boolean
}>({
handle: {
stop () {
refs.handle?.stop()
},
ready () {
refs.updateOnReady = true
return refs.handle?.ready()
}
},
updateOnReady: false
})
const useSubscriptionClient = (
name: string | false,
args: any[]
rijk marked this conversation as resolved.
Show resolved Hide resolved
) => {
const subscription = useRef<Meteor.SubscriptionHandle>()

useEffect(() => {
if (!name) {
subscription.current = null
return
}

// Use Tracker.nonreactive in case we are inside a Tracker Computation.
// This can happen if someone calls `ReactDOM.render` inside a Computation.
// In that case, we want to opt out of the normal behavior of nested
// Computations, where if the outer one is invalidated or stopped,
// it stops the inner one.
const computation = Tracker.nonreactive(() => (
Tracker.autorun(() => {
refs.handle = factory()
if (!refs.handle) return
if (refs.updateOnReady && refs.handle.ready()) {
forceUpdate()
}
subscription.current = Meteor.subscribe( name, ...args )
// @ts-ignore this is just an internal thing
subscription.current.deps = { name, args }
})
))

return () => {
computation.stop()
}
}, deps)
return () => computation.stop()
}, [name, ...args])

return useCallback(
() => {
if (subscription.current) {
// @ts-ignore
const { deps } = subscription.current
if (deps.name === name && deps.args === args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you don't need to manually track and compare deps here, you can just set subscription.current back to null in the effect cleanup phase ?

Copy link
Author

Choose a reason for hiding this comment

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

Well, the thing is, this all happens after the render. So it will already be too late by then.

Copy link
Contributor

Choose a reason for hiding this comment

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

The cleanup phase runs after the render, but before the ready() callback you return gets executed I think ?
Then by the time it gets executed, subscription.current has gone back to null and you can just return false ?
(not 100% sure off the top of my head, but worth a try I think)

More importantly, this code still has the same drawback as before : by not running the reactive subscription.current.ready() in some cases, the function we build here won't be reactive later ?

I have something in mind, need to try it first :-)

Copy link
Author

Choose a reason for hiding this comment

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

The cleanup phase runs after the render, but before the ready() callback you return gets executed I think ?
Then by the time it gets executed, subscription.current has gone back to null and you can just return false ?

Nope. Tried 10 different ways, even with useState, but every time the first ready() call still references the old subscription.

return subscription.current.ready()
} else {
// Prevented returning the previous subscription's status
}
}

return refs.handle
return false
},
[name, ...args]
)
}

const useSubscriptionServer = (): Meteor.SubscriptionHandle => ({
stop() {},
ready() { return true }
})

export const useSubscription = (factory: () => Meteor.SubscriptionHandle | void, deps: DependencyList = []) => (
export const useSubscription = (name: string | false, ...args: any[]) => (
Meteor.isServer
? useSubscriptionServer()
: useSubscriptionClient(factory, deps)
: useSubscriptionClient(name, args)
)

const useCursorClient = <T = any>(factory: () => Mongo.Cursor<T>, deps: DependencyList = []) => {
Expand Down
3 changes: 1 addition & 2 deletions packages/react-mongo/types/react-mongo.d.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { Meteor } from 'meteor/meteor';
import { Mongo } from 'meteor/mongo';
import { DependencyList } from 'react';
declare type UseSubscriptionOptions = {
deps?: DependencyList;
updateOnReady?: boolean;
};
export declare const useSubscription: (factory: () => Meteor.SubscriptionHandle | void, deps?: DependencyList | UseSubscriptionOptions) => void;
export declare const useSubscription: (name: string | false, args: any[]) => () => boolean;
export declare const useCursor: <T = any>(factory: () => Mongo.Cursor<T>, deps?: DependencyList) => Mongo.Cursor<T>;
export {};