From e63cbd031ce780f18d062facf78b41b7e783c5ef Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Thu, 18 Jul 2019 15:02:51 -0400 Subject: [PATCH 01/34] Implement a computationHandler API, with a cleanup method --- packages/react-meteor-data/useTracker.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index fff0af45..8ad4df3f 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -73,12 +73,16 @@ function areHookInputsEqual(nextDeps, prevDeps) { let uniqueCounter = 0; -function useTracker(reactiveFn, deps) { +function useTracker(reactiveFn, deps, computationHandler) { const { current: refs } = useRef({}); const [, forceUpdate] = useState(); const dispose = () => { + if (refs.computationCleanup) { + refs.computationCleanup(); + delete refs.computationCleanup; + } if (refs.computation) { refs.computation.stop(); refs.computation = null; @@ -110,6 +114,11 @@ function useTracker(reactiveFn, deps) { }; if (c.firstRun) { + // If there is a computationHandler, pass it the computation, and store the + // result, which may be a cleanup method. + if (computationHandler) { + refs.computationCleanup = computationHandler(c); + } // This will capture data synchronously on first run (and after deps change). // Additional cycles will follow the normal computation behavior. runReactiveFn(); From c9d2e2f2de4eeaf32c58f292649b570df738fde4 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Thu, 18 Jul 2019 17:14:48 -0400 Subject: [PATCH 02/34] use a self contained forceUpdate value --- packages/react-meteor-data/useTracker.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index fff0af45..8ca6c487 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -71,12 +71,10 @@ function areHookInputsEqual(nextDeps, prevDeps) { return true; } -let uniqueCounter = 0; - function useTracker(reactiveFn, deps) { const { current: refs } = useRef({}); - const [, forceUpdate] = useState(); + const [counter, forceUpdate] = useState(0); const dispose = () => { if (refs.computation) { @@ -121,7 +119,7 @@ function useTracker(reactiveFn, deps) { runReactiveFn(); } // use a uniqueCounter to trigger a state change to force a re-render - forceUpdate(++uniqueCounter); + forceUpdate(counter + 1); } }) )); From 1315b55c11912da6a39969c7d3afa8e2f9c098c7 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Fri, 19 Jul 2019 12:42:55 -0400 Subject: [PATCH 03/34] Add a check and a warning for the appropriate return type from computationHandler --- packages/react-meteor-data/useTracker.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 8ad4df3f..acd70615 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -117,7 +117,16 @@ function useTracker(reactiveFn, deps, computationHandler) { // If there is a computationHandler, pass it the computation, and store the // result, which may be a cleanup method. if (computationHandler) { - refs.computationCleanup = computationHandler(c); + const cleanupHandler = computationHandler(c); + if (cleanupHandler) { + if (Meteor.isDevelopment && cleanupHandler !== 'function') { + warn( + 'Warning: Computation handler should only return a function ' + + 'to be used for cleanup, and never return any other value.' + ); + } + refs.computationCleanup = cleanupHandler; + } } // This will capture data synchronously on first run (and after deps change). // Additional cycles will follow the normal computation behavior. From bb29e321301c28e235e9709e4c73fa6c372bf35c Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Fri, 19 Jul 2019 13:48:53 -0400 Subject: [PATCH 04/34] Rewrite readme for brevity and to update and explain optional `deps` --- packages/react-meteor-data/README.md | 38 ++++++++++++++++------------ 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/packages/react-meteor-data/README.md b/packages/react-meteor-data/README.md index e4d953c4..c75a837b 100644 --- a/packages/react-meteor-data/README.md +++ b/packages/react-meteor-data/README.md @@ -22,19 +22,19 @@ This package provides two ways to use Tracker reactive data in your React compon - a hook: `useTracker` (v2 only, requires React `^16.8`) - a higher-order component (HOC): `withTracker` (v1 and v2). -The `useTracker` hook, introduced in version 2.0.0, is slightly more straightforward to use (lets you access reactive data sources directly within your componenent, rather than adding them from an external wrapper), and slightly more performant (avoids adding wrapper layers in the React tree). But, like all React hooks, it can only be used in function components, not in class components. -The `withTracker` HOC can be used with all components, function or class. +The `useTracker` hook, introduced in version 2.0.0, embraces the [benefits of hooks](https://reactjs.org/docs/hooks-faq.html). Like all React hooks, it can only be used in function components, not in class components. -It is not necessary to rewrite existing applications to use the `useTracker` hook instead of the existing `withTracker` HOC. But for new components, it is suggested to prefer the `useTracker` hook when dealing with function components. +The `withTracker` HOC can be used with all components, function or class based. + +It is not necessary to rewrite existing applications to use the `useTracker` hook instead of the existing `withTracker` HOC. #### `useTracker(reactiveFn, deps)` hook You can use the `useTracker` hook to get the value of a Tracker reactive function in your (function) components. The reactive function will get re-run whenever its reactive inputs change, and the component will re-render with the new value. Arguments: -- `reactiveFn`: a Tracker reactive function (with no parameters) -- `deps`: an array of "dependencies" of the reactive function, i.e. the list of values that, when changed, need to stop the current Tracker computation and start a new one - for example, the value of a prop used in a subscription or a Minimongo query; see example below. This array typically includes all variables from the outer scope "captured" in the closure passed as the 1st argument. This is very similar to how the `deps` argument for [React's built-in `useEffect`, `useCallback` or `useMemo` hooks](https://reactjs.org/docs/hooks-reference.html) work. -If omitted, the Tracker computation will be recreated on every call. +- `reactiveFn`: A Tracker reactive function (with no parameters). +- `deps`: An optional array of "dependencies" of the reactive function. This is very similar to how the `deps` argument for [React's built-in `useEffect`, `useCallback` or `useMemo` hooks](https://reactjs.org/docs/hooks-reference.html) work. If omitted, the Tracker computation will be recreated on every render (Note: `withTracker` has always done this). If provided, the computation will be retained, and reactive updates after the first run will run asynchronously from the react render cycle. This array typically includes all variables from the outer scope "captured" in the closure passed as the 1st argument. For example, the value of a prop used in a subscription or a Minimongo query; see example below. ```js import { useTracker } from 'meteor/react-meteor-data'; @@ -42,14 +42,20 @@ import { useTracker } from 'meteor/react-meteor-data'; // React function component. function Foo({ listId }) { // This computation uses no value from the outer scope, - // and thus does not needs to pass a 'deps' argument (same as passing []). - const currentUser = useTracker(() => Meteor.user()); - // The following two computations both depend on the 'listId' prop, - // and thus need to specify it in the 'deps' argument, - // in order to subscribe to the expected 'todoList' subscription - // or fetch the expected Tasks when the 'listId' prop changes. + // and thus does not needs to pass a 'deps' argument. + // However, we can optimize the use of the computation + // by providing an empty deps array. With it, the + // computation will be retained instead of torn down and + // rebuilt on every render. useTracker will produce the + // same results either way. + const currentUser = useTracker(() => Meteor.user(), []); + + // The following two computations both depend on the + // listId prop. When deps are specified, the computation + // will be retained. const listLoading = useTracker(() => { - // Note that this subscription will get cleaned up when your component is unmounted. + // Note that this subscription will get cleaned up + // when your component is unmounted or deps change. const handle = Meteor.subscribe('todoList', listId); return !handle.ready(); }, [listId]); @@ -119,9 +125,9 @@ For more information, see the [React article](http://guide.meteor.com/react.html - `useTracker` hook + `withTracker` HOC - Requires React `^16.8`. - Implementation is compatible with the forthcoming "React Suspense" features. - - The `withTracker` HOC is strictly backwards-compatible with the one provided in v1.x, the major version number is only motivated by the bump of React version requirement. -Provided they use a compatible React version, existing Meteor apps leveraging the `withTracker` HOC can freely upgrade from v1.x to v2.x, and gain compatibility with future React versions. - + - The `withTracker` HOC is strictly backwards-compatible with the one provided in v1.x, the major version number is only motivated by the bump of React version requirement. Provided a compatible React version, existing Meteor apps leveraging the `withTracker` HOC can freely upgrade from v1.x to v2.x, and gain compatibility with future React versions. + - The previously deprecated `createContainer` has been removed. + - `react-meteor-data` v1.x / v0.x : - `withTracker` HOC (+ `createContainer`, kept for backwards compatibility with early v0.x releases) - Requires React `^15.3` or `^16.0`. From 574416a6ff0d00c5a789faa783b3bb644f909c23 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Fri, 19 Jul 2019 14:59:57 -0400 Subject: [PATCH 05/34] Add some basic tests for useTracker --- packages/react-meteor-data/index.js | 1 + packages/react-meteor-data/package-lock.json | 132 ++++++++++++++++++ packages/react-meteor-data/package.js | 10 +- packages/react-meteor-data/package.json | 9 ++ packages/react-meteor-data/tests.js | 1 + .../react-meteor-data/useTracker.tests.js | 77 ++++++++++ 6 files changed, 229 insertions(+), 1 deletion(-) create mode 100644 packages/react-meteor-data/package-lock.json create mode 100644 packages/react-meteor-data/package.json create mode 100644 packages/react-meteor-data/tests.js create mode 100644 packages/react-meteor-data/useTracker.tests.js diff --git a/packages/react-meteor-data/index.js b/packages/react-meteor-data/index.js index bf99268a..8769794b 100644 --- a/packages/react-meteor-data/index.js +++ b/packages/react-meteor-data/index.js @@ -1,3 +1,4 @@ +/* global Meteor*/ import React from 'react'; if (Meteor.isDevelopment) { diff --git a/packages/react-meteor-data/package-lock.json b/packages/react-meteor-data/package-lock.json new file mode 100644 index 00000000..1e4c7b2e --- /dev/null +++ b/packages/react-meteor-data/package-lock.json @@ -0,0 +1,132 @@ +{ + "name": "react-meteor-data", + "requires": true, + "lockfileVersion": 1, + "dependencies": { + "@babel/runtime": { + "version": "7.5.5", + "resolved": "https://registry.npmjs.org/@babel/runtime/-/runtime-7.5.5.tgz", + "integrity": "sha512-28QvEGyQyNkB0/m2B4FU7IEZGK2NUrcMtT6BZEFALTguLk+AUT6ofsHtPk5QyjAdUkpMJ+/Em+quwz4HOt30AQ==", + "requires": { + "regenerator-runtime": "^0.13.2" + } + }, + "@testing-library/react-hooks": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/@testing-library/react-hooks/-/react-hooks-1.1.0.tgz", + "integrity": "sha512-piE/ceQoNf134FFVXBABDbttBJ8eLPD4eg7zIciVJv92RyvoIsBHCvvG8Vd4IG5pyuWYrkLsZTO8ucZBwa4twA==", + "requires": { + "@babel/runtime": "^7.4.2", + "@types/react": "^16.8.22", + "@types/react-test-renderer": "^16.8.2" + } + }, + "@types/prop-types": { + "version": "15.7.1", + "resolved": "https://registry.npmjs.org/@types/prop-types/-/prop-types-15.7.1.tgz", + "integrity": "sha512-CFzn9idOEpHrgdw8JsoTkaDDyRWk1jrzIV8djzcgpq0y9tG4B4lFT+Nxh52DVpDXV+n4+NPNv7M1Dj5uMp6XFg==" + }, + "@types/react": { + "version": "16.8.23", + "resolved": "https://registry.npmjs.org/@types/react/-/react-16.8.23.tgz", + "integrity": "sha512-abkEOIeljniUN9qB5onp++g0EY38h7atnDHxwKUFz1r3VH1+yG1OKi2sNPTyObL40goBmfKFpdii2lEzwLX1cA==", + "requires": { + "@types/prop-types": "*", + "csstype": "^2.2.0" + } + }, + "@types/react-test-renderer": { + "version": "16.8.2", + "resolved": "https://registry.npmjs.org/@types/react-test-renderer/-/react-test-renderer-16.8.2.tgz", + "integrity": "sha512-cm42QR9S9V3aOxLh7Fh7PUqQ8oSfSdnSni30T7TiTmlKvE6aUlo+LhQAzjnZBPriD9vYmgG8MXI8UDK4Nfb7gg==", + "requires": { + "@types/react": "*" + } + }, + "csstype": { + "version": "2.6.6", + "resolved": "https://registry.npmjs.org/csstype/-/csstype-2.6.6.tgz", + "integrity": "sha512-RpFbQGUE74iyPgvr46U9t1xoQBM8T4BL8SxrN66Le2xYAPSaDJJKeztV3awugusb3g3G9iL8StmkBBXhcbbXhg==" + }, + "js-tokens": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/js-tokens/-/js-tokens-4.0.0.tgz", + "integrity": "sha512-RdJUflcE3cUzKiMqQgsCu06FPu9UdIJO0beYbPhHN4k6apgJtifcoCtT9bcxOpYBtpD2kCM6Sbzg4CausW/PKQ==" + }, + "loose-envify": { + "version": "1.4.0", + "resolved": "https://registry.npmjs.org/loose-envify/-/loose-envify-1.4.0.tgz", + "integrity": "sha512-lyuxPGr/Wfhrlem2CL/UcnUc1zcqKAImBDzukY7Y5F/yQiNdko6+fRLevlw1HgMySw7f611UIY408EtxRSoK3Q==", + "requires": { + "js-tokens": "^3.0.0 || ^4.0.0" + } + }, + "object-assign": { + "version": "4.1.1", + "resolved": "https://registry.npmjs.org/object-assign/-/object-assign-4.1.1.tgz", + "integrity": "sha1-IQmtx5ZYh8/AXLvUQsrIv7s2CGM=" + }, + "prop-types": { + "version": "15.7.2", + "resolved": "https://registry.npmjs.org/prop-types/-/prop-types-15.7.2.tgz", + "integrity": "sha512-8QQikdH7//R2vurIJSutZ1smHYTcLpRWEOlHnzcWHmBYrOGUysKwSsrC89BCiFj3CbrfJ/nXFdJepOVrY1GCHQ==", + "requires": { + "loose-envify": "^1.4.0", + "object-assign": "^4.1.1", + "react-is": "^16.8.1" + } + }, + "react": { + "version": "16.8.6", + "resolved": "https://registry.npmjs.org/react/-/react-16.8.6.tgz", + "integrity": "sha512-pC0uMkhLaHm11ZSJULfOBqV4tIZkx87ZLvbbQYunNixAAvjnC+snJCg0XQXn9VIsttVsbZP/H/ewzgsd5fxKXw==", + "requires": { + "loose-envify": "^1.1.0", + "object-assign": "^4.1.1", + "prop-types": "^15.6.2", + "scheduler": "^0.13.6" + } + }, + "react-dom": { + "version": "16.8.6", + "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-16.8.6.tgz", + "integrity": "sha512-1nL7PIq9LTL3fthPqwkvr2zY7phIPjYrT0jp4HjyEQrEROnw4dG41VVwi/wfoCneoleqrNX7iAD+pXebJZwrwA==", + "requires": { + "loose-envify": "^1.1.0", + "object-assign": "^4.1.1", + "prop-types": "^15.6.2", + "scheduler": "^0.13.6" + } + }, + "react-is": { + "version": "16.8.6", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.8.6.tgz", + "integrity": "sha512-aUk3bHfZ2bRSVFFbbeVS4i+lNPZr3/WM5jT2J5omUVV1zzcs1nAaf3l51ctA5FFvCRbhrH0bdAsRRQddFJZPtA==" + }, + "react-test-renderer": { + "version": "16.8.6", + "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.8.6.tgz", + "integrity": "sha512-H2srzU5IWYT6cZXof6AhUcx/wEyJddQ8l7cLM/F7gDXYyPr4oq+vCIxJYXVGhId1J706sqziAjuOEjyNkfgoEw==", + "requires": { + "object-assign": "^4.1.1", + "prop-types": "^15.6.2", + "react-is": "^16.8.6", + "scheduler": "^0.13.6" + } + }, + "regenerator-runtime": { + "version": "0.13.3", + "resolved": "https://registry.npmjs.org/regenerator-runtime/-/regenerator-runtime-0.13.3.tgz", + "integrity": "sha512-naKIZz2GQ8JWh///G7L3X6LaQUAMp2lvb1rvwwsURe/VXwD6VMfr+/1NuNw3ag8v2kY1aQ/go5SNn79O9JU7yw==" + }, + "scheduler": { + "version": "0.13.6", + "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.13.6.tgz", + "integrity": "sha512-IWnObHt413ucAYKsD9J1QShUKkbKLQQHdxRyw73sw4FN26iWr3DY/H34xGPe4nmL1DwXyWmSWmMrA9TfQbE/XQ==", + "requires": { + "loose-envify": "^1.1.0", + "object-assign": "^4.1.1" + } + } + } +} diff --git a/packages/react-meteor-data/package.js b/packages/react-meteor-data/package.js index 80df8ff5..ebd1987d 100644 --- a/packages/react-meteor-data/package.js +++ b/packages/react-meteor-data/package.js @@ -1,3 +1,5 @@ +/* global Package */ + Package.describe({ name: 'react-meteor-data', summary: 'React higher-order component for reactively tracking Meteor data', @@ -6,10 +8,16 @@ Package.describe({ git: 'https://github.com/meteor/react-packages', }); -Package.onUse(function (api) { +Package.onUse((api) => { api.versionsFrom('1.3'); api.use('tracker'); api.use('ecmascript'); api.mainModule('index.js'); }); + +Package.onTest((api) => { + api.use(['ecmascript', 'reactive-dict', 'tracker', 'tinytest']); + api.use('react-meteor-data'); + api.mainModule('tests.js'); +}); diff --git a/packages/react-meteor-data/package.json b/packages/react-meteor-data/package.json new file mode 100644 index 00000000..7514840f --- /dev/null +++ b/packages/react-meteor-data/package.json @@ -0,0 +1,9 @@ +{ + "name": "react-meteor-data", + "dependencies": { + "react": "16.8.6", + "react-dom": "16.8.6", + "react-test-renderer": "16.8.6", + "@testing-library/react-hooks": "1.1.0" + } +} diff --git a/packages/react-meteor-data/tests.js b/packages/react-meteor-data/tests.js new file mode 100644 index 00000000..db53dbe6 --- /dev/null +++ b/packages/react-meteor-data/tests.js @@ -0,0 +1 @@ +import './useTracker.tests.js'; diff --git a/packages/react-meteor-data/useTracker.tests.js b/packages/react-meteor-data/useTracker.tests.js new file mode 100644 index 00000000..5dcdb378 --- /dev/null +++ b/packages/react-meteor-data/useTracker.tests.js @@ -0,0 +1,77 @@ +/* global Tinytest */ +import React from 'react'; +import { renderHook, act } from '@testing-library/react-hooks'; +import { ReactiveDict } from 'meteor/reactive-dict'; + +import useTracker from './useTracker'; + +Tinytest.add('useTracker - no deps', async function (test) { + const reactiveDict = new ReactiveDict('test1', { key: 'initial' }); + let renderCount = 0; + + const { result, rerender, unmount, waitForNextUpdate } = renderHook( + () => useTracker(() => { + renderCount++; + return reactiveDict.get('key'); + }) + ); + + test.equal(result.current, 'initial', 'Expect initial value to be "initial"'); + test.equal(renderCount, 1, 'Should run rendered 1 times'); + + if (Meteor.isServer) return; + + act(() => reactiveDict.set('key', 'changed')); + await waitForNextUpdate(); + + test.equal(result.current, 'changed', 'Expect new value to be "changed"'); + test.equal(renderCount, 2, 'Should run rendered 2 times'); + + rerender(); + + test.equal(result.current, 'changed', 'Expect value of "changed" to persist after rerender'); + test.equal(renderCount, 3, 'Should run rendered 3 times'); + + unmount(); + reactiveDict.destroy(); + test.equal(renderCount, 3, 'Should run rendered 3 times'); +}); + +Tinytest.add('useTracker - with deps', async function (test) { + const reactiveDict = new ReactiveDict('test2', {}); + let renderCount = 0; + + const { result, rerender, unmount, waitForNextUpdate } = renderHook( + ({ name }) => useTracker(() => { + renderCount++; + reactiveDict.setDefault(name, 'default'); + return reactiveDict.get(name); + }, [name]), + { initialProps: { name: 'name' } } + ); + + test.equal(result.current, 'default', 'Expect the default value for given name to be "default"'); + test.equal(renderCount, 1, 'Should run rendered 1 times'); + + if (Meteor.isServer) return; + + act(() => reactiveDict.set('name', 'changed')); + await waitForNextUpdate(); + + test.equal(result.current, 'changed', 'Expect the new value for given name to be "changed"'); + test.equal(renderCount, 2, 'Should run rendered 2 times'); + + rerender(); + + test.equal(result.current, 'changed', 'Expect the new value "changed" for given name to have persisted through render'); + test.equal(renderCount, 3, 'Should run rendered 3 times'); + + rerender({ name: 'different' }); + + test.equal(result.current, 'default', 'After deps change, the default value should have returned'); + test.equal(renderCount, 4, 'Should run rendered 4 times'); + + unmount(); + reactiveDict.destroy(); + test.equal(renderCount, 4, 'Should run rendered 4 times'); +}); From 796ce6d3cbae451e200f236047954936a0ecac40 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Fri, 19 Jul 2019 15:18:20 -0400 Subject: [PATCH 06/34] add some after unmount tests --- .../react-meteor-data/useTracker.tests.js | 42 +++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/packages/react-meteor-data/useTracker.tests.js b/packages/react-meteor-data/useTracker.tests.js index 5dcdb378..5f8d0549 100644 --- a/packages/react-meteor-data/useTracker.tests.js +++ b/packages/react-meteor-data/useTracker.tests.js @@ -7,17 +7,17 @@ import useTracker from './useTracker'; Tinytest.add('useTracker - no deps', async function (test) { const reactiveDict = new ReactiveDict('test1', { key: 'initial' }); - let renderCount = 0; + let runCount = 0; const { result, rerender, unmount, waitForNextUpdate } = renderHook( () => useTracker(() => { - renderCount++; + runCount++; return reactiveDict.get('key'); }) ); test.equal(result.current, 'initial', 'Expect initial value to be "initial"'); - test.equal(renderCount, 1, 'Should run rendered 1 times'); + test.equal(runCount, 1, 'Should have run 1 times'); if (Meteor.isServer) return; @@ -25,25 +25,32 @@ Tinytest.add('useTracker - no deps', async function (test) { await waitForNextUpdate(); test.equal(result.current, 'changed', 'Expect new value to be "changed"'); - test.equal(renderCount, 2, 'Should run rendered 2 times'); + test.equal(runCount, 2, 'Should have run 2 times'); rerender(); test.equal(result.current, 'changed', 'Expect value of "changed" to persist after rerender'); - test.equal(renderCount, 3, 'Should run rendered 3 times'); + test.equal(runCount, 3, 'Should have run 3 times'); unmount(); + test.equal(runCount, 3, 'Unmount should not cause a tracker run'); + + act(() => reactiveDict.set('different', 'changed again')); + // we can't use await waitForNextUpdate() here because it doesn't trigger re-render - is there a way to test that? + + test.equal(result.current, 'default', 'After unmount, changes to the reactive source should not update the value.'); + test.equal(runCount, 3, 'After unmount, useTracker should no longer be tracking'); + reactiveDict.destroy(); - test.equal(renderCount, 3, 'Should run rendered 3 times'); }); Tinytest.add('useTracker - with deps', async function (test) { const reactiveDict = new ReactiveDict('test2', {}); - let renderCount = 0; + let runCount = 0; const { result, rerender, unmount, waitForNextUpdate } = renderHook( ({ name }) => useTracker(() => { - renderCount++; + runCount++; reactiveDict.setDefault(name, 'default'); return reactiveDict.get(name); }, [name]), @@ -51,7 +58,7 @@ Tinytest.add('useTracker - with deps', async function (test) { ); test.equal(result.current, 'default', 'Expect the default value for given name to be "default"'); - test.equal(renderCount, 1, 'Should run rendered 1 times'); + test.equal(runCount, 1, 'Should have run 1 times'); if (Meteor.isServer) return; @@ -59,19 +66,28 @@ Tinytest.add('useTracker - with deps', async function (test) { await waitForNextUpdate(); test.equal(result.current, 'changed', 'Expect the new value for given name to be "changed"'); - test.equal(renderCount, 2, 'Should run rendered 2 times'); + test.equal(runCount, 2, 'Should have run 2 times'); rerender(); + await waitForNextUpdate(); test.equal(result.current, 'changed', 'Expect the new value "changed" for given name to have persisted through render'); - test.equal(renderCount, 3, 'Should run rendered 3 times'); + test.equal(runCount, 3, 'Should have run 3 times'); rerender({ name: 'different' }); + await waitForNextUpdate(); test.equal(result.current, 'default', 'After deps change, the default value should have returned'); - test.equal(renderCount, 4, 'Should run rendered 4 times'); + test.equal(runCount, 4, 'Should have run 4 times'); unmount(); + test.equal(runCount, 4, 'Unmount should not cause a tracker run'); + // we can't use await waitForNextUpdate() here because it doesn't trigger re-render - is there a way to test that? + + act(() => reactiveDict.set('different', 'changed again')); + + test.equal(result.current, 'default', 'After unmount, changes to the reactive source should not update the value.'); + test.equal(runCount, 4, 'After unmount, useTracker should no longer be tracking'); + reactiveDict.destroy(); - test.equal(renderCount, 4, 'Should run rendered 4 times'); }); From f0df78a443080a6f137cd6bd3a9bdc267fff2455 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Fri, 19 Jul 2019 15:18:55 -0400 Subject: [PATCH 07/34] make sure we aren't accidentally triggering reactiveDict's migration framework --- packages/react-meteor-data/useTracker.tests.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/react-meteor-data/useTracker.tests.js b/packages/react-meteor-data/useTracker.tests.js index 5f8d0549..b0b8b868 100644 --- a/packages/react-meteor-data/useTracker.tests.js +++ b/packages/react-meteor-data/useTracker.tests.js @@ -6,7 +6,8 @@ import { ReactiveDict } from 'meteor/reactive-dict'; import useTracker from './useTracker'; Tinytest.add('useTracker - no deps', async function (test) { - const reactiveDict = new ReactiveDict('test1', { key: 'initial' }); + const reactiveDict = new ReactiveDict(); + reactiveDict.setDefault('key', 'initial') let runCount = 0; const { result, rerender, unmount, waitForNextUpdate } = renderHook( @@ -45,7 +46,7 @@ Tinytest.add('useTracker - no deps', async function (test) { }); Tinytest.add('useTracker - with deps', async function (test) { - const reactiveDict = new ReactiveDict('test2', {}); + const reactiveDict = new ReactiveDict(); let runCount = 0; const { result, rerender, unmount, waitForNextUpdate } = renderHook( From 0c1dd3c4835e89d7248a666f5f79d8eea0d43bc3 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Fri, 19 Jul 2019 15:40:29 -0400 Subject: [PATCH 08/34] Test changes to enclosed values when not using deps too --- .../react-meteor-data/useTracker.tests.js | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/react-meteor-data/useTracker.tests.js b/packages/react-meteor-data/useTracker.tests.js index b0b8b868..c0060e2f 100644 --- a/packages/react-meteor-data/useTracker.tests.js +++ b/packages/react-meteor-data/useTracker.tests.js @@ -7,21 +7,20 @@ import useTracker from './useTracker'; Tinytest.add('useTracker - no deps', async function (test) { const reactiveDict = new ReactiveDict(); - reactiveDict.setDefault('key', 'initial') let runCount = 0; const { result, rerender, unmount, waitForNextUpdate } = renderHook( - () => useTracker(() => { + ({ name }) => useTracker(() => { runCount++; - return reactiveDict.get('key'); - }) + reactiveDict.setDefault(name, 'initial'); + return reactiveDict.get(name); + }), + { initialProps: { name: 'key' } } ); test.equal(result.current, 'initial', 'Expect initial value to be "initial"'); test.equal(runCount, 1, 'Should have run 1 times'); - if (Meteor.isServer) return; - act(() => reactiveDict.set('key', 'changed')); await waitForNextUpdate(); @@ -29,18 +28,25 @@ Tinytest.add('useTracker - no deps', async function (test) { test.equal(runCount, 2, 'Should have run 2 times'); rerender(); + await waitForNextUpdate(); test.equal(result.current, 'changed', 'Expect value of "changed" to persist after rerender'); test.equal(runCount, 3, 'Should have run 3 times'); + rerender({ name: 'different' }); + await waitForNextUpdate(); + + test.equal(result.current, 'default', 'After deps change, the default value should have returned'); + test.equal(runCount, 4, 'Should have run 4 times'); + unmount(); - test.equal(runCount, 3, 'Unmount should not cause a tracker run'); + test.equal(runCount, 4, 'Unmount should not cause a tracker run'); act(() => reactiveDict.set('different', 'changed again')); // we can't use await waitForNextUpdate() here because it doesn't trigger re-render - is there a way to test that? test.equal(result.current, 'default', 'After unmount, changes to the reactive source should not update the value.'); - test.equal(runCount, 3, 'After unmount, useTracker should no longer be tracking'); + test.equal(runCount, 4, 'After unmount, useTracker should no longer be tracking'); reactiveDict.destroy(); }); @@ -61,8 +67,6 @@ Tinytest.add('useTracker - with deps', async function (test) { test.equal(result.current, 'default', 'Expect the default value for given name to be "default"'); test.equal(runCount, 1, 'Should have run 1 times'); - if (Meteor.isServer) return; - act(() => reactiveDict.set('name', 'changed')); await waitForNextUpdate(); From 68ec9c953939b0872f250f80ce93eebfd48686e7 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Fri, 19 Jul 2019 22:38:24 -0400 Subject: [PATCH 09/34] Allow setting deps in withTracker options --- packages/react-meteor-data/withTracker.jsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-meteor-data/withTracker.jsx b/packages/react-meteor-data/withTracker.jsx index e6e6a9f2..6eee1ba3 100644 --- a/packages/react-meteor-data/withTracker.jsx +++ b/packages/react-meteor-data/withTracker.jsx @@ -4,10 +4,10 @@ import useTracker from './useTracker.js'; export default function withTracker(options) { return Component => { const expandedOptions = typeof options === 'function' ? { getMeteorData: options } : options; - const { getMeteorData, pure = true } = expandedOptions; + const { getMeteorData, pure = true, deps = null } = expandedOptions; const WithTracker = forwardRef((props, ref) => { - const data = useTracker(() => getMeteorData(props) || {}); + const data = useTracker(() => getMeteorData(props) || {}, deps); return ; }); From a4ce87dad31d4b5f4adfa4fbcc998d01d538b2d8 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Fri, 19 Jul 2019 22:48:41 -0400 Subject: [PATCH 10/34] add missing typeof operator --- packages/react-meteor-data/useTracker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 2cab12c6..d3393018 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -117,7 +117,7 @@ function useTracker(reactiveFn, deps, computationHandler) { if (computationHandler) { const cleanupHandler = computationHandler(c); if (cleanupHandler) { - if (Meteor.isDevelopment && cleanupHandler !== 'function') { + if (Meteor.isDevelopment && typeof cleanupHandler !== 'function') { warn( 'Warning: Computation handler should only return a function ' + 'to be used for cleanup, and never return any other value.' From f44809fd9ee5881000040be776331ef4818a5bfc Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Sat, 20 Jul 2019 12:18:57 -0400 Subject: [PATCH 11/34] Use the full hook on the server, to allow complete computation lifecycle --- packages/react-meteor-data/useTracker.js | 104 ++++++++++------------- 1 file changed, 46 insertions(+), 58 deletions(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index d3393018..8b6ce1a8 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -98,72 +98,60 @@ function useTracker(reactiveFn, deps, computationHandler) { // store the deps for comparison on next render refs.previousDeps = deps; - // 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. - refs.computation = Tracker.nonreactive(() => ( - Tracker.autorun((c) => { - const runReactiveFn = () => { - const data = reactiveFn(); - if (Meteor.isDevelopment) checkCursor(data); - refs.trackerData = data; - }; - - if (c.firstRun) { - // If there is a computationHandler, pass it the computation, and store the - // result, which may be a cleanup method. - if (computationHandler) { - const cleanupHandler = computationHandler(c); - if (cleanupHandler) { - if (Meteor.isDevelopment && typeof cleanupHandler !== 'function') { - warn( - 'Warning: Computation handler should only return a function ' - + 'to be used for cleanup, and never return any other value.' - ); - } - refs.computationCleanup = cleanupHandler; + const tracked = (c) => { + const runReactiveFn = () => { + const data = reactiveFn(); + if (Meteor.isDevelopment) checkCursor(data); + refs.trackerData = data; + }; + + if (c === null || c.firstRun) { + // If there is a computationHandler, pass it the computation, and store the + // result, which may be a cleanup method. + if (computationHandler) { + const cleanupHandler = computationHandler(c); + if (cleanupHandler) { + if (Meteor.isDevelopment && typeof cleanupHandler !== 'function') { + warn( + 'Warning: Computation handler should return a function ' + + 'to be used for cleanup or nothing.' + ); } + refs.computationCleanup = cleanupHandler; } - // This will capture data synchronously on first run (and after deps change). - // Additional cycles will follow the normal computation behavior. - runReactiveFn(); + } + // This will capture data synchronously on first run (and after deps change). + // Additional cycles will follow the normal computation behavior. + runReactiveFn(); + } else { + // If deps are falsy, stop computation and let next render handle reactiveFn. + if (!refs.previousDeps) { + dispose(); } else { - // If deps are falsy, stop computation and let next render handle reactiveFn. - if (!refs.previousDeps) { - dispose(); - } else { - runReactiveFn(); - } - // use a uniqueCounter to trigger a state change to force a re-render - forceUpdate(counter + 1); + runReactiveFn(); } - }) - )); - } + // use a uniqueCounter to trigger a state change to force a re-render + forceUpdate(counter + 1); + } + } - // stop the computation on unmount only - useEffect(() => { - if (Meteor.isDevelopment - && deps !== null && deps !== undefined - && !Array.isArray(deps)) { - warn( - 'Warning: useTracker expected an initial dependency value of ' - + `type array but got type of ${typeof deps} instead.` - ); + // When rendering on the server, we don't want to use the Tracker. + if (Meteor.isServer) { + refs.computation = tracked(null); + } else { + // 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. + refs.computation = Tracker.nonreactive(() => Tracker.autorun(tracked)); } + } - return dispose; - }, []); + // stop the computation on unmount + useEffect(() => dispose, []); return refs.trackerData; } -// When rendering on the server, we don't want to use the Tracker. -// We only do the first rendering on the server so we can get the data right away -function useTrackerServer(reactiveFn) { - return reactiveFn(); -} - -export default (Meteor.isServer ? useTrackerServer : useTracker); +export default useTracker; From 85fb4844e36f6643cd7a9b0b4ddb13e0585a2fb1 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Sat, 20 Jul 2019 12:19:36 -0400 Subject: [PATCH 12/34] Add some argument checks and move the deps check to the same block --- packages/react-meteor-data/useTracker.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 8b6ce1a8..199b341c 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -72,6 +72,27 @@ function areHookInputsEqual(nextDeps, prevDeps) { } function useTracker(reactiveFn, deps, computationHandler) { + if (Meteor.isDevelopment) { + if (typeof reactiveFn !== 'function') { + warn( + `Warning: useTracker expected a function in it's first argument ` + + `(reactiveFn), but got type of ${typeof reactiveFn}.` + ) + } + if (deps && !Array.isArray(deps)) { + warn( + `Warning: useTracker expected an array in it's second argument ` + + `(dependency), but got type of ${typeof deps}.` + ); + } + if (typeof computationHandler !== 'function') { + warn( + `Warning: useTracker expected a function in it's third argument` + + `(computationHandler), but got type of ${typeof computationHandler}.` + ); + } + } + const { current: refs } = useRef({}); const [counter, forceUpdate] = useState(0); From 9975c73e3e73cb6a82993cca3abbb69f33f09c2f Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Sat, 20 Jul 2019 12:20:06 -0400 Subject: [PATCH 13/34] Add tests for the computation lifecycle --- .../react-meteor-data/useTracker.tests.js | 47 ++++++++++++++++++- 1 file changed, 45 insertions(+), 2 deletions(-) diff --git a/packages/react-meteor-data/useTracker.tests.js b/packages/react-meteor-data/useTracker.tests.js index c0060e2f..b476c70b 100644 --- a/packages/react-meteor-data/useTracker.tests.js +++ b/packages/react-meteor-data/useTracker.tests.js @@ -8,45 +8,66 @@ import useTracker from './useTracker'; Tinytest.add('useTracker - no deps', async function (test) { const reactiveDict = new ReactiveDict(); let runCount = 0; + let computation; + let createdCount = 0; + let destroyedCount = 0; const { result, rerender, unmount, waitForNextUpdate } = renderHook( ({ name }) => useTracker(() => { runCount++; reactiveDict.setDefault(name, 'initial'); return reactiveDict.get(name); + }, null, (c) => { + computation = c; + createdCount++; + return () => { + destroyedCount++; + } }), { initialProps: { name: 'key' } } ); test.equal(result.current, 'initial', 'Expect initial value to be "initial"'); test.equal(runCount, 1, 'Should have run 1 times'); + test.equal(createdCount, 1, 'Should have been created 1 times'); + test.equal(destroyedCount, 0, 'Should not have been destroyed yet'); act(() => reactiveDict.set('key', 'changed')); await waitForNextUpdate(); test.equal(result.current, 'changed', 'Expect new value to be "changed"'); test.equal(runCount, 2, 'Should have run 2 times'); + test.equal(createdCount, 2, 'Should have been created 2 times'); + test.equal(destroyedCount, 1, 'Should have been destroyed 1 less than created'); rerender(); await waitForNextUpdate(); test.equal(result.current, 'changed', 'Expect value of "changed" to persist after rerender'); test.equal(runCount, 3, 'Should have run 3 times'); + test.equal(createdCount, 3, 'Should have been created 3 times'); + test.equal(destroyedCount, 2, 'Should have been destroyed 1 less than created'); rerender({ name: 'different' }); await waitForNextUpdate(); test.equal(result.current, 'default', 'After deps change, the default value should have returned'); test.equal(runCount, 4, 'Should have run 4 times'); + test.equal(createdCount, 4, 'Should have been created 4 times'); + test.equal(destroyedCount, 3, 'Should have been destroyed 1 less than created'); unmount(); test.equal(runCount, 4, 'Unmount should not cause a tracker run'); + test.equal(createdCount, 4, 'Should have been created 4 times'); + test.equal(destroyedCount, 4, 'Should have been destroyed the same number of times as created'); act(() => reactiveDict.set('different', 'changed again')); // we can't use await waitForNextUpdate() here because it doesn't trigger re-render - is there a way to test that? test.equal(result.current, 'default', 'After unmount, changes to the reactive source should not update the value.'); test.equal(runCount, 4, 'After unmount, useTracker should no longer be tracking'); + test.equal(createdCount, 4, 'Should have been created 4 times'); + test.equal(destroyedCount, 4, 'Should have been destroyed the same number of times as created'); reactiveDict.destroy(); }); @@ -54,45 +75,67 @@ Tinytest.add('useTracker - no deps', async function (test) { Tinytest.add('useTracker - with deps', async function (test) { const reactiveDict = new ReactiveDict(); let runCount = 0; + let computation; + let createdCount = 0; + let destroyedCount = 0; const { result, rerender, unmount, waitForNextUpdate } = renderHook( ({ name }) => useTracker(() => { runCount++; reactiveDict.setDefault(name, 'default'); return reactiveDict.get(name); - }, [name]), + }, [name], (c) => { + test.isFalse(c === computation, 'The new computation should always be a new instance'); + computation = c; + createdCount++; + return () => { + destroyedCount++; + } + }), { initialProps: { name: 'name' } } ); test.equal(result.current, 'default', 'Expect the default value for given name to be "default"'); test.equal(runCount, 1, 'Should have run 1 times'); + test.equal(createdCount, 1, 'Should have been created 1 times'); + test.equal(destroyedCount, 0, 'Should not have been destroyed yet'); act(() => reactiveDict.set('name', 'changed')); await waitForNextUpdate(); test.equal(result.current, 'changed', 'Expect the new value for given name to be "changed"'); test.equal(runCount, 2, 'Should have run 2 times'); + test.equal(createdCount, 1, 'Should have been created 1 times'); + test.equal(destroyedCount, 0, 'Should not have been destroyed yet'); rerender(); await waitForNextUpdate(); test.equal(result.current, 'changed', 'Expect the new value "changed" for given name to have persisted through render'); test.equal(runCount, 3, 'Should have run 3 times'); + test.equal(createdCount, 1, 'Should have been created 1 times'); + test.equal(destroyedCount, 0, 'Should not have been destroyed yet'); rerender({ name: 'different' }); await waitForNextUpdate(); test.equal(result.current, 'default', 'After deps change, the default value should have returned'); test.equal(runCount, 4, 'Should have run 4 times'); + test.equal(createdCount, 2, 'Should have been created 2 times'); + test.equal(destroyedCount, 1, 'Should have been destroyed 1 times'); unmount(); - test.equal(runCount, 4, 'Unmount should not cause a tracker run'); // we can't use await waitForNextUpdate() here because it doesn't trigger re-render - is there a way to test that? + test.equal(runCount, 4, 'Unmount should not cause a tracker run'); + test.equal(createdCount, 2, 'Should have been created 2 times'); + test.equal(destroyedCount, 2, 'Should have been destroyed 2 times'); act(() => reactiveDict.set('different', 'changed again')); test.equal(result.current, 'default', 'After unmount, changes to the reactive source should not update the value.'); test.equal(runCount, 4, 'After unmount, useTracker should no longer be tracking'); + test.equal(createdCount, 2, 'Should have been created 2 times'); + test.equal(destroyedCount, 2, 'Should have been destroyed 2 times'); reactiveDict.destroy(); }); From 7de2ea65b5223f4b1bf4516ebe08945333bd53a3 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Sat, 20 Jul 2019 12:51:09 -0400 Subject: [PATCH 14/34] Fix forceUpdate by using a reference to update the counter value --- packages/react-meteor-data/useTracker.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 8ca6c487..f4c5fd43 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -75,6 +75,7 @@ function useTracker(reactiveFn, deps) { const { current: refs } = useRef({}); const [counter, forceUpdate] = useState(0); + refs.counter = counter const dispose = () => { if (refs.computation) { @@ -119,7 +120,7 @@ function useTracker(reactiveFn, deps) { runReactiveFn(); } // use a uniqueCounter to trigger a state change to force a re-render - forceUpdate(counter + 1); + forceUpdate(refs.counter + 1); } }) )); From 9e59a83c69a30971b3aa3fa5de19c101dfd49ddc Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Sat, 20 Jul 2019 13:50:53 -0400 Subject: [PATCH 15/34] add a comment explaining the use of a reference with forceUpdate and the counter var --- packages/react-meteor-data/useTracker.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index f4c5fd43..4112c768 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -119,7 +119,10 @@ function useTracker(reactiveFn, deps) { } else { runReactiveFn(); } - // use a uniqueCounter to trigger a state change to force a re-render + // Increment a reference to counter to trigger a state change to force a re-render + // Since this computation callback is reused, we'll need to make sure to access the + // counter value from a reference instead of using the enclosed value, so we can + // get the value of any updates. forceUpdate(refs.counter + 1); } }) From bff72c7c291743acd5348f81846ea2679000666c Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Sun, 21 Jul 2019 15:02:42 -0400 Subject: [PATCH 16/34] Use a much cleaner forceUpdate method based on useReducer --- packages/react-meteor-data/useTracker.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 4112c768..cf442365 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -1,5 +1,5 @@ /* global Meteor, Package, Tracker */ -import React, { useState, useEffect, useRef } from 'react'; +import React, { useReducer, useEffect, useRef } from 'react'; // Use React.warn() if available (should ship in React 16.9). const warn = React.warn || console.warn.bind(console); @@ -71,11 +71,14 @@ function areHookInputsEqual(nextDeps, prevDeps) { return true; } +// Used to create a forceUpdate from useReducer. Forces update by +// incrementing a number whenever the dispatch method is invoked. +const fur = x => x + 1; + function useTracker(reactiveFn, deps) { const { current: refs } = useRef({}); - const [counter, forceUpdate] = useState(0); - refs.counter = counter + const [, forceUpdate] = useReducer(fur, 0); const dispose = () => { if (refs.computation) { @@ -119,11 +122,7 @@ function useTracker(reactiveFn, deps) { } else { runReactiveFn(); } - // Increment a reference to counter to trigger a state change to force a re-render - // Since this computation callback is reused, we'll need to make sure to access the - // counter value from a reference instead of using the enclosed value, so we can - // get the value of any updates. - forceUpdate(refs.counter + 1); + forceUpdate(); } }) )); From b9d69114b052a3a58a78f33479762e5e5be90864 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Mon, 22 Jul 2019 10:58:57 -0400 Subject: [PATCH 17/34] Update to avoid running side effects in render, cause double invocation of reactiveFn on mount, possibly more with suspense --- packages/react-meteor-data/useTracker.js | 48 ++++++++++++++---------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index cf442365..13cade04 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -87,6 +87,12 @@ function useTracker(reactiveFn, deps) { } }; + const runReactiveFn = () => { + const data = reactiveFn(); + if (Meteor.isDevelopment) checkCursor(data); + refs.trackerData = data; + }; + // this is called like at componentWillMount and componentWillUpdate equally // in order to support render calls with synchronous data from the reactive computation // if prevDeps or deps are not set areHookInputsEqual always returns false @@ -95,8 +101,30 @@ function useTracker(reactiveFn, deps) { // if we are re-creating the computation, we need to stop the old one. dispose(); + // No side-effects are allowed when computing the initial value. + // To get the initial return value for the 1st render on mount, + // we run reactiveFn without autorun or subscriptions. + // Note: maybe when React Suspense is officially available we could + // throw a Promise instead to skip the 1st render altogether ? + const realSubscribe = Meteor.subscribe; + Meteor.subscribe = () => ({ stop: () => {}, ready: () => false }); + Tracker.nonreactive(runReactiveFn); + Meteor.subscribe = realSubscribe; + // store the deps for comparison on next render refs.previousDeps = deps; + } + + // stop the computation on unmount only + useEffect(() => { + if (Meteor.isDevelopment + && deps !== null && deps !== undefined + && !Array.isArray(deps)) { + warn( + 'Warning: useTracker expected an initial dependency value of ' + + `type array but got type of ${typeof deps} instead.` + ); + } // Use Tracker.nonreactive in case we are inside a Tracker Computation. // This can happen if someone calls `ReactDOM.render` inside a Computation. @@ -105,12 +133,6 @@ function useTracker(reactiveFn, deps) { // it stops the inner one. refs.computation = Tracker.nonreactive(() => ( Tracker.autorun((c) => { - const runReactiveFn = () => { - const data = reactiveFn(); - if (Meteor.isDevelopment) checkCursor(data); - refs.trackerData = data; - }; - if (c.firstRun) { // This will capture data synchronously on first run (and after deps change). // Additional cycles will follow the normal computation behavior. @@ -126,21 +148,9 @@ function useTracker(reactiveFn, deps) { } }) )); - } - - // stop the computation on unmount only - useEffect(() => { - if (Meteor.isDevelopment - && deps !== null && deps !== undefined - && !Array.isArray(deps)) { - warn( - 'Warning: useTracker expected an initial dependency value of ' - + `type array but got type of ${typeof deps} instead.` - ); - } return dispose; - }, []); + }, deps); return refs.trackerData; } From 7e71790ead404da65b959a31cbc825178afcbc37 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Mon, 22 Jul 2019 14:33:29 -0400 Subject: [PATCH 18/34] Revert "Update to avoid running side effects in render, cause double invocation of reactiveFn on mount, possibly more with suspense" This reverts commit b9d69114b052a3a58a78f33479762e5e5be90864. --- packages/react-meteor-data/useTracker.js | 48 ++++++++++-------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 13cade04..cf442365 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -87,12 +87,6 @@ function useTracker(reactiveFn, deps) { } }; - const runReactiveFn = () => { - const data = reactiveFn(); - if (Meteor.isDevelopment) checkCursor(data); - refs.trackerData = data; - }; - // this is called like at componentWillMount and componentWillUpdate equally // in order to support render calls with synchronous data from the reactive computation // if prevDeps or deps are not set areHookInputsEqual always returns false @@ -101,30 +95,8 @@ function useTracker(reactiveFn, deps) { // if we are re-creating the computation, we need to stop the old one. dispose(); - // No side-effects are allowed when computing the initial value. - // To get the initial return value for the 1st render on mount, - // we run reactiveFn without autorun or subscriptions. - // Note: maybe when React Suspense is officially available we could - // throw a Promise instead to skip the 1st render altogether ? - const realSubscribe = Meteor.subscribe; - Meteor.subscribe = () => ({ stop: () => {}, ready: () => false }); - Tracker.nonreactive(runReactiveFn); - Meteor.subscribe = realSubscribe; - // store the deps for comparison on next render refs.previousDeps = deps; - } - - // stop the computation on unmount only - useEffect(() => { - if (Meteor.isDevelopment - && deps !== null && deps !== undefined - && !Array.isArray(deps)) { - warn( - 'Warning: useTracker expected an initial dependency value of ' - + `type array but got type of ${typeof deps} instead.` - ); - } // Use Tracker.nonreactive in case we are inside a Tracker Computation. // This can happen if someone calls `ReactDOM.render` inside a Computation. @@ -133,6 +105,12 @@ function useTracker(reactiveFn, deps) { // it stops the inner one. refs.computation = Tracker.nonreactive(() => ( Tracker.autorun((c) => { + const runReactiveFn = () => { + const data = reactiveFn(); + if (Meteor.isDevelopment) checkCursor(data); + refs.trackerData = data; + }; + if (c.firstRun) { // This will capture data synchronously on first run (and after deps change). // Additional cycles will follow the normal computation behavior. @@ -148,9 +126,21 @@ function useTracker(reactiveFn, deps) { } }) )); + } + + // stop the computation on unmount only + useEffect(() => { + if (Meteor.isDevelopment + && deps !== null && deps !== undefined + && !Array.isArray(deps)) { + warn( + 'Warning: useTracker expected an initial dependency value of ' + + `type array but got type of ${typeof deps} instead.` + ); + } return dispose; - }, deps); + }, []); return refs.trackerData; } From 5d73c1a9d5e9840aee55807656ba7458d6f3dfd4 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Mon, 22 Jul 2019 19:04:30 -0400 Subject: [PATCH 19/34] Make "falsy" deps check in computation consistent with other checks. --- packages/react-meteor-data/useTracker.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index cf442365..c05108ca 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -117,7 +117,8 @@ function useTracker(reactiveFn, deps) { runReactiveFn(); } else { // If deps are falsy, stop computation and let next render handle reactiveFn. - if (!refs.previousDeps) { + if (!refs.previousDeps !== null && refs.previousDeps !== undefined + && !Array.isArray(refs.previousDeps)) { dispose(); } else { runReactiveFn(); From 044e16349fd4dee3b7c0a972b001af142054a159 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Mon, 22 Jul 2019 23:42:40 -0400 Subject: [PATCH 20/34] Port the old outdated tests to useTracker (all tests are without deps) --- packages/react-meteor-data/package.js | 2 +- .../react-meteor-data/useTracker.tests.js | 381 +++++++++++++++++- 2 files changed, 381 insertions(+), 2 deletions(-) diff --git a/packages/react-meteor-data/package.js b/packages/react-meteor-data/package.js index ebd1987d..030d3e32 100644 --- a/packages/react-meteor-data/package.js +++ b/packages/react-meteor-data/package.js @@ -17,7 +17,7 @@ Package.onUse((api) => { }); Package.onTest((api) => { - api.use(['ecmascript', 'reactive-dict', 'tracker', 'tinytest']); + api.use(['ecmascript', 'reactive-dict', 'reactive-var', 'tracker', 'tinytest', 'underscore']); api.use('react-meteor-data'); api.mainModule('tests.js'); }); diff --git a/packages/react-meteor-data/useTracker.tests.js b/packages/react-meteor-data/useTracker.tests.js index c0060e2f..19e9ad83 100644 --- a/packages/react-meteor-data/useTracker.tests.js +++ b/packages/react-meteor-data/useTracker.tests.js @@ -1,7 +1,10 @@ /* global Tinytest */ -import React from 'react'; +import React, { useState } from 'react'; +import ReactDOM from 'react-dom'; + import { renderHook, act } from '@testing-library/react-hooks'; import { ReactiveDict } from 'meteor/reactive-dict'; +import { ReactiveVar } from 'meteor/reactive-var'; import useTracker from './useTracker'; @@ -96,3 +99,379 @@ Tinytest.add('useTracker - with deps', async function (test) { reactiveDict.destroy(); }); + +const canonicalizeHtml = function(html) { + var h = html; + // kill IE-specific comments inserted by DomRange + h = h.replace(//g, ''); + h = h.replace(//g, ''); + // ignore exact text of comments + h = h.replace(//g, ''); + // make all tags lowercase + h = h.replace(/<\/?(\w+)/g, function(m) { + return m.toLowerCase(); }); + // replace whitespace sequences with spaces + h = h.replace(/\s+/g, ' '); + // Trim leading and trailing whitespace + h = h.replace(/^\s+|\s+$/g, ''); + // remove whitespace before and after tags + h = h.replace(/\s*(<\/?\w.*?>)\s*/g, function (m, tag) { + return tag; }); + // make tag attributes uniform + h = h.replace(/<(\w+)\s+(.*?)\s*>/g, function(m, tagName, attrs) { + // Drop expando property used by Sizzle (part of jQuery) which leaks into + // attributes in IE8. Note that its value always contains spaces. + attrs = attrs.replace(/sizcache[0-9]+="[^"]*"/g, ' '); + // Similarly for expando properties used by jQuery to track data. + attrs = attrs.replace(/jQuery[0-9]+="[0-9]+"/g, ' '); + // Similarly for expando properties used to DOMBackend to keep + // track of callbacks to fire when an element is removed + attrs = attrs.replace(/\$blaze_teardown_callbacks="[^"]*"/g, ' '); + // And by DOMRange to keep track of the element's DOMRange + attrs = attrs.replace(/\$blaze_range="[^"]*"/g, ' '); + + attrs = attrs.replace(/\s*=\s*/g, '='); + attrs = attrs.replace(/^\s+/g, ''); + attrs = attrs.replace(/\s+$/g, ''); + attrs = attrs.replace(/\s+/g, ' '); + // quote unquoted attribute values, as in `type=checkbox`. This + // will do the wrong thing if there's an `=` in an attribute value. + attrs = attrs.replace(/(\w)=([^'" >/]+)/g, '$1="$2"'); + + // for the purpose of splitting attributes in a string like 'a="b" + // c="d"', assume they are separated by a single space and values + // are double- or single-quoted, but allow for spaces inside the + // quotes. Split on space following quote. + var attrList = attrs.replace(/(\w)='([^']*)' /g, "$1='$2'\u0000"); + attrList = attrList.replace(/(\w)="([^"]*)" /g, '$1="$2"\u0000'); + attrList = attrList.split("\u0000"); + // put attributes in alphabetical order + attrList.sort(); + + var tagContents = [tagName]; + + for(var i=0; i'; + }); + return h; +}; + +const getInnerHtml = function (elem) { + // clean up elem.innerHTML and strip data-reactid attributes too + return canonicalizeHtml(elem.innerHTML).replace(/ data-reactroot=".*?"/g, ''); +}; + +if (Meteor.isClient) { + Tinytest.add('react-meteor-data - basic track', function (test) { + var div = document.createElement("DIV"); + + var x = new ReactiveVar('aaa'); + + var Foo = () => { + const data = useTracker(() => { + return { + x: x.get() + }; + }) + return {data.x}; + }; + + ReactDOM.render(, div); + test.equal(getInnerHtml(div), 'aaa'); + + x.set('bbb'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'bbb'); + + test.equal(x._numListeners(), 1); + + ReactDOM.unmountComponentAtNode(div); + + test.equal(x._numListeners(), 0); + }); + + // Make sure that calling ReactDOM.render() from an autorun doesn't + // associate that autorun with the mixin's autorun. When autoruns are + // nested, invalidating the outer one stops the inner one, unless + // Tracker.nonreactive is used. This test tests for the use of + // Tracker.nonreactive around the mixin's autorun. + Tinytest.add('react-meteor-data - render in autorun', function (test) { + var div = document.createElement("DIV"); + + var x = new ReactiveVar('aaa'); + + var Foo = () => { + const data = useTracker(() => { + return { + x: x.get() + }; + }); + return {data.x}; + }; + + Tracker.autorun(function (c) { + ReactDOM.render(, div); + // Stopping this autorun should not affect the mixin's autorun. + c.stop(); + }); + test.equal(getInnerHtml(div), 'aaa'); + + x.set('bbb'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'bbb'); + + ReactDOM.unmountComponentAtNode(div); + }); + + Tinytest.add('react-meteor-data - track based on props and state', function (test) { + var div = document.createElement("DIV"); + + var xs = [new ReactiveVar('aaa'), + new ReactiveVar('bbb'), + new ReactiveVar('ccc')]; + + let setState; + var Foo = (props) => { + const [state, _setState] = useState({ m: 0 }); + setState = _setState; + const data = useTracker(() => { + return { + x: xs[state.m + props.n].get() + }; + }); + return {data.x}; + }; + + var comp = ReactDOM.render(, div); + + test.equal(getInnerHtml(div), 'aaa'); + xs[0].set('AAA'); + test.equal(getInnerHtml(div), 'aaa'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'AAA'); + + { + let comp2 = ReactDOM.render(, div); + test.isTrue(comp === comp2); + } + + test.equal(getInnerHtml(div), 'bbb'); + xs[1].set('BBB'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'BBB'); + + setState({m: 1}); + test.equal(getInnerHtml(div), 'ccc'); + xs[2].set('CCC'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'CCC'); + + ReactDOM.render(, div); + setState({m: 0}); + test.equal(getInnerHtml(div), 'AAA'); + + ReactDOM.unmountComponentAtNode(div); + }); + + function waitFor(func, callback) { + Tracker.autorun(function (c) { + if (func()) { + c.stop(); + callback(); + } + }); + }; + + testAsyncMulti('react-meteor-data - resubscribe', [ + function (test, expect) { + var self = this; + self.div = document.createElement("DIV"); + self.collection = new Mongo.Collection("react-meteor-data-mixin-coll"); + self.num = new ReactiveVar(1); + self.someOtherVar = new ReactiveVar('foo'); + self.Foo = () => { + const data = useTracker(() => { + self.handle = + Meteor.subscribe("react-meteor-data-mixin-sub", + self.num.get()); + + return { + v: self.someOtherVar.get(), + docs: self.collection.find().fetch() + }; + }); + return
{ + _.map(data.docs, (doc) => {doc._id}) + }
; + }; + + self.component = ReactDOM.render(, self.div); + test.equal(getInnerHtml(self.div), '
'); + + var handle = self.handle; + test.isFalse(handle.ready()); + + waitFor(() => handle.ready(), + expect()); + }, + function (test, expect) { + var self = this; + test.isTrue(self.handle.ready()); + test.equal(getInnerHtml(self.div), '
id1
'); + + self.someOtherVar.set('bar'); + self.oldHandle1 = self.handle; + + // can't call Tracker.flush() here (we are in a Tracker.flush already) + Tracker.afterFlush(expect()); + }, + function (test, expect) { + var self = this; + var oldHandle = self.oldHandle1; + var newHandle = self.handle; + test.notEqual(oldHandle, newHandle); // new handle + test.equal(newHandle.subscriptionId, oldHandle.subscriptionId); // same sub + test.isTrue(newHandle.ready()); // doesn't become unready + // no change to the content + test.equal(getInnerHtml(self.div), '
id1
'); + + // ok, now change the `num` argument to the subscription + self.num.set(2); + self.oldHandle2 = newHandle; + Tracker.afterFlush(expect()); + }, + function (test, expect) { + var self = this; + // data is still there + test.equal(getInnerHtml(self.div), '
id1
'); + // handle is no longer ready + var handle = self.component.handle; + test.isFalse(handle.ready()); + // different sub ID + test.isTrue(self.oldHandle2.subscriptionId); + test.isTrue(handle.subscriptionId); + test.notEqual(handle.subscriptionId, self.oldHandle2.subscriptionId); + + waitFor(() => handle.ready(), + expect()); + }, + function (test, expect) { + var self = this; + // now we see the new data! (and maybe the old data, because + // when a subscription goes away, its data doesn't disappear right + // away; the server has to tell the client which documents or which + // properties to remove, and this is not easy to wait for either; see + // https://github.com/meteor/meteor/issues/2440) + test.equal(getInnerHtml(self.div).replace('id1', ''), + '
id2
'); + + self.someOtherVar.set('baz'); + self.oldHandle3 = self.component.handle; + + Tracker.afterFlush(expect()); + }, + function (test, expect) { + var self = this; + test.equal(self.component.data.v, 'baz'); + test.notEqual(self.oldHandle3, self.component.handle); + test.equal(self.oldHandle3.subscriptionId, + self.component.handle.subscriptionId); + test.isTrue(self.component.handle.ready()); + }, + function (test, expect) { + ReactDOM.unmountComponentAtNode(this.div); + // break out of flush time, so we don't call the test's + // onComplete from within Tracker.flush + Meteor.defer(expect()); + } + ]); + + Tinytest.add( + "react-meteor-data - print warning if return cursor from getMeteorData", + function (test) { + var coll = new Mongo.Collection(null); + var ComponentWithCursor = React.createClass({ + mixins: [ReactMeteorData], + getMeteorData() { + return { + theCursor: coll.find() + }; + }, + render() { + return ; + } + }); + + // Check if we print a warning to console about props + // You can be sure this test is correct because we have an identical one in + // react-runtime-dev + let warning; + try { + var oldWarn = console.warn; + console.warn = function specialWarn(message) { + warning = message; + }; + + var div = document.createElement("DIV"); + ReactDOM.render(, div); + + test.matches(warning, /cursor from getMeteorData/); + } finally { + console.warn = oldWarn; + } + }); + +} else { + Meteor.publish("react-meteor-data-mixin-sub", function (num) { + Meteor.defer(() => { // because subs are blocking + this.added("react-meteor-data-mixin-coll", 'id'+num, {}); + this.ready(); + }); +}); + +} From 884246c302af7964f6b371bf1caaaa4799cba336 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Tue, 23 Jul 2019 00:25:57 -0400 Subject: [PATCH 21/34] get more ported tests working --- packages/react-meteor-data/package.js | 3 +- .../react-meteor-data/useTracker.tests.js | 179 ++++-------------- 2 files changed, 40 insertions(+), 142 deletions(-) diff --git a/packages/react-meteor-data/package.js b/packages/react-meteor-data/package.js index 030d3e32..49c2a0dd 100644 --- a/packages/react-meteor-data/package.js +++ b/packages/react-meteor-data/package.js @@ -17,7 +17,8 @@ Package.onUse((api) => { }); Package.onTest((api) => { - api.use(['ecmascript', 'reactive-dict', 'reactive-var', 'tracker', 'tinytest', 'underscore']); + api.use(['ecmascript', 'reactive-dict', 'reactive-var', 'tracker', 'tinytest', 'underscore', 'mongo']); + api.use('test-helpers'); api.use('react-meteor-data'); api.mainModule('tests.js'); }); diff --git a/packages/react-meteor-data/useTracker.tests.js b/packages/react-meteor-data/useTracker.tests.js index 19e9ad83..6c136729 100644 --- a/packages/react-meteor-data/useTracker.tests.js +++ b/packages/react-meteor-data/useTracker.tests.js @@ -100,107 +100,6 @@ Tinytest.add('useTracker - with deps', async function (test) { reactiveDict.destroy(); }); -const canonicalizeHtml = function(html) { - var h = html; - // kill IE-specific comments inserted by DomRange - h = h.replace(//g, ''); - h = h.replace(//g, ''); - // ignore exact text of comments - h = h.replace(//g, ''); - // make all tags lowercase - h = h.replace(/<\/?(\w+)/g, function(m) { - return m.toLowerCase(); }); - // replace whitespace sequences with spaces - h = h.replace(/\s+/g, ' '); - // Trim leading and trailing whitespace - h = h.replace(/^\s+|\s+$/g, ''); - // remove whitespace before and after tags - h = h.replace(/\s*(<\/?\w.*?>)\s*/g, function (m, tag) { - return tag; }); - // make tag attributes uniform - h = h.replace(/<(\w+)\s+(.*?)\s*>/g, function(m, tagName, attrs) { - // Drop expando property used by Sizzle (part of jQuery) which leaks into - // attributes in IE8. Note that its value always contains spaces. - attrs = attrs.replace(/sizcache[0-9]+="[^"]*"/g, ' '); - // Similarly for expando properties used by jQuery to track data. - attrs = attrs.replace(/jQuery[0-9]+="[0-9]+"/g, ' '); - // Similarly for expando properties used to DOMBackend to keep - // track of callbacks to fire when an element is removed - attrs = attrs.replace(/\$blaze_teardown_callbacks="[^"]*"/g, ' '); - // And by DOMRange to keep track of the element's DOMRange - attrs = attrs.replace(/\$blaze_range="[^"]*"/g, ' '); - - attrs = attrs.replace(/\s*=\s*/g, '='); - attrs = attrs.replace(/^\s+/g, ''); - attrs = attrs.replace(/\s+$/g, ''); - attrs = attrs.replace(/\s+/g, ' '); - // quote unquoted attribute values, as in `type=checkbox`. This - // will do the wrong thing if there's an `=` in an attribute value. - attrs = attrs.replace(/(\w)=([^'" >/]+)/g, '$1="$2"'); - - // for the purpose of splitting attributes in a string like 'a="b" - // c="d"', assume they are separated by a single space and values - // are double- or single-quoted, but allow for spaces inside the - // quotes. Split on space following quote. - var attrList = attrs.replace(/(\w)='([^']*)' /g, "$1='$2'\u0000"); - attrList = attrList.replace(/(\w)="([^"]*)" /g, '$1="$2"\u0000'); - attrList = attrList.split("\u0000"); - // put attributes in alphabetical order - attrList.sort(); - - var tagContents = [tagName]; - - for(var i=0; i'; - }); - return h; -}; - const getInnerHtml = function (elem) { // clean up elem.innerHTML and strip data-reactid attributes too return canonicalizeHtml(elem.innerHTML).replace(/ data-reactroot=".*?"/g, ''); @@ -345,6 +244,7 @@ if (Meteor.isClient) { docs: self.collection.find().fetch() }; }); + self.data = data; return
{ _.map(data.docs, (doc) => {doc._id}) }
; @@ -390,7 +290,7 @@ if (Meteor.isClient) { // data is still there test.equal(getInnerHtml(self.div), '
id1
'); // handle is no longer ready - var handle = self.component.handle; + var handle = self.handle; test.isFalse(handle.ready()); // different sub ID test.isTrue(self.oldHandle2.subscriptionId); @@ -411,17 +311,17 @@ if (Meteor.isClient) { '
id2
'); self.someOtherVar.set('baz'); - self.oldHandle3 = self.component.handle; + self.oldHandle3 = self.handle; Tracker.afterFlush(expect()); }, function (test, expect) { var self = this; - test.equal(self.component.data.v, 'baz'); - test.notEqual(self.oldHandle3, self.component.handle); + test.equal(self.data.v, 'baz'); + test.notEqual(self.oldHandle3, self.handle); test.equal(self.oldHandle3.subscriptionId, - self.component.handle.subscriptionId); - test.isTrue(self.component.handle.ready()); + self.handle.subscriptionId); + test.isTrue(self.handle.ready()); }, function (test, expect) { ReactDOM.unmountComponentAtNode(this.div); @@ -431,40 +331,37 @@ if (Meteor.isClient) { } ]); - Tinytest.add( - "react-meteor-data - print warning if return cursor from getMeteorData", - function (test) { - var coll = new Mongo.Collection(null); - var ComponentWithCursor = React.createClass({ - mixins: [ReactMeteorData], - getMeteorData() { - return { - theCursor: coll.find() - }; - }, - render() { - return ; - } - }); - - // Check if we print a warning to console about props - // You can be sure this test is correct because we have an identical one in - // react-runtime-dev - let warning; - try { - var oldWarn = console.warn; - console.warn = function specialWarn(message) { - warning = message; - }; - - var div = document.createElement("DIV"); - ReactDOM.render(, div); - - test.matches(warning, /cursor from getMeteorData/); - } finally { - console.warn = oldWarn; - } - }); + // Tinytest.add( + // "react-meteor-data - print warning if return cursor from useTracker", + // function (test) { + // var coll = new Mongo.Collection(null); + // var ComponentWithCursor = () => { + // useTracker(() => { + // return { + // theCursor: coll.find() + // }; + // }); + // return ; + // }; + + // // Check if we print a warning to console about props + // // You can be sure this test is correct because we have an identical one in + // // react-runtime-dev + // let warning; + // try { + // var oldWarn = console.warn; + // console.warn = function specialWarn(message) { + // warning = message; + // }; + + // var div = document.createElement("DIV"); + // ReactDOM.render(, div); + + // test.matches(warning, /cursor before returning it/); + // } finally { + // console.warn = oldWarn; + // } + // }); } else { Meteor.publish("react-meteor-data-mixin-sub", function (num) { From 4976a3b6bb50b6bce1ee399d69cdd5de20b69595 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Tue, 23 Jul 2019 00:26:20 -0400 Subject: [PATCH 22/34] add a version of one of the ported tests with deps --- .../react-meteor-data/useTracker.tests.js | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/packages/react-meteor-data/useTracker.tests.js b/packages/react-meteor-data/useTracker.tests.js index 6c136729..d20ad42c 100644 --- a/packages/react-meteor-data/useTracker.tests.js +++ b/packages/react-meteor-data/useTracker.tests.js @@ -217,6 +217,56 @@ if (Meteor.isClient) { ReactDOM.unmountComponentAtNode(div); }); + Tinytest.add('react-meteor-data - track based on props and state (with deps)', function (test) { + var div = document.createElement("DIV"); + + var xs = [new ReactiveVar('aaa'), + new ReactiveVar('bbb'), + new ReactiveVar('ccc')]; + + let setState; + var Foo = (props) => { + const [state, _setState] = useState({ m: 0 }); + setState = _setState; + const data = useTracker(() => { + return { + x: xs[state.m + props.n].get() + }; + }, [state.m, props.n]); + return {data.x}; + }; + + var comp = ReactDOM.render(, div); + + test.equal(getInnerHtml(div), 'aaa'); + xs[0].set('AAA'); + test.equal(getInnerHtml(div), 'aaa'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'AAA'); + + { + let comp2 = ReactDOM.render(, div); + test.isTrue(comp === comp2); + } + + test.equal(getInnerHtml(div), 'bbb'); + xs[1].set('BBB'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'BBB'); + + setState({m: 1}); + test.equal(getInnerHtml(div), 'ccc'); + xs[2].set('CCC'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'CCC'); + + ReactDOM.render(, div); + setState({m: 0}); + test.equal(getInnerHtml(div), 'AAA'); + + ReactDOM.unmountComponentAtNode(div); + }); + function waitFor(func, callback) { Tracker.autorun(function (c) { if (func()) { From 774350c82e76ec521ddf9fbea908bb2e0d9c4c2f Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Tue, 23 Jul 2019 00:52:44 -0400 Subject: [PATCH 23/34] Add withTracker tests --- packages/react-meteor-data/tests.js | 1 + .../react-meteor-data/withTracker.tests.js | 335 ++++++++++++++++++ 2 files changed, 336 insertions(+) create mode 100644 packages/react-meteor-data/withTracker.tests.js diff --git a/packages/react-meteor-data/tests.js b/packages/react-meteor-data/tests.js index db53dbe6..e444261a 100644 --- a/packages/react-meteor-data/tests.js +++ b/packages/react-meteor-data/tests.js @@ -1 +1,2 @@ import './useTracker.tests.js'; +import './withTracker.tests.js'; diff --git a/packages/react-meteor-data/withTracker.tests.js b/packages/react-meteor-data/withTracker.tests.js new file mode 100644 index 00000000..c4e44ccb --- /dev/null +++ b/packages/react-meteor-data/withTracker.tests.js @@ -0,0 +1,335 @@ +/* global Tinytest */ +import React, { useState } from 'react'; +import ReactDOM from 'react-dom'; + +import { renderHook, act } from '@testing-library/react-hooks'; +import { ReactiveDict } from 'meteor/reactive-dict'; +import { ReactiveVar } from 'meteor/reactive-var'; + +import withTracker from './withTracker'; + +const getInnerHtml = function (elem) { + // clean up elem.innerHTML and strip data-reactid attributes too + return canonicalizeHtml(elem.innerHTML).replace(/ data-reactroot=".*?"/g, ''); +}; + +if (Meteor.isClient) { + Tinytest.add('withTracker - basic track', function (test) { + var div = document.createElement("DIV"); + + var x = new ReactiveVar('aaa'); + + var Foo = withTracker(() => { + return { + x: x.get() + }; + })((props) => { + return {props.x}; + }); + + ReactDOM.render(, div); + test.equal(getInnerHtml(div), 'aaa'); + + x.set('bbb'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'bbb'); + + test.equal(x._numListeners(), 1); + + ReactDOM.unmountComponentAtNode(div); + + test.equal(x._numListeners(), 0); + }); + + // Make sure that calling ReactDOM.render() from an autorun doesn't + // associate that autorun with the mixin's autorun. When autoruns are + // nested, invalidating the outer one stops the inner one, unless + // Tracker.nonreactive is used. This test tests for the use of + // Tracker.nonreactive around the mixin's autorun. + Tinytest.add('withTracker - render in autorun', function (test) { + var div = document.createElement("DIV"); + + var x = new ReactiveVar('aaa'); + + var Foo = withTracker(() => { + return { + x: x.get() + }; + })((props) => { + return {props.x}; + }); + + Tracker.autorun(function (c) { + ReactDOM.render(, div); + // Stopping this autorun should not affect the mixin's autorun. + c.stop(); + }); + test.equal(getInnerHtml(div), 'aaa'); + + x.set('bbb'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'bbb'); + + ReactDOM.unmountComponentAtNode(div); + }); + + Tinytest.add('withTracker - track based on props and state', function (test) { + var div = document.createElement("DIV"); + + var xs = [new ReactiveVar('aaa'), + new ReactiveVar('bbb'), + new ReactiveVar('ccc')]; + + let setState; + var Foo = (props) => { + const [state, _setState] = useState({ m: 0 }); + setState = _setState; + const Component = withTracker((props) => { + return { + x: xs[state.m + props.n].get() + }; + })((props) => { + return {props.x}; + }); + return + }; + + var comp = ReactDOM.render(, div); + + test.equal(getInnerHtml(div), 'aaa'); + xs[0].set('AAA'); + test.equal(getInnerHtml(div), 'aaa'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'AAA'); + + { + let comp2 = ReactDOM.render(, div); + test.isTrue(comp === comp2); + } + + test.equal(getInnerHtml(div), 'bbb'); + xs[1].set('BBB'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'BBB'); + + setState({m: 1}); + test.equal(getInnerHtml(div), 'ccc'); + xs[2].set('CCC'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'CCC'); + + ReactDOM.render(, div); + setState({m: 0}); + test.equal(getInnerHtml(div), 'AAA'); + + ReactDOM.unmountComponentAtNode(div); + }); + + Tinytest.add('withTracker - track based on props and state (with deps)', function (test) { + var div = document.createElement("DIV"); + + var xs = [new ReactiveVar('aaa'), + new ReactiveVar('bbb'), + new ReactiveVar('ccc')]; + + let setState; + var Foo = (props) => { + const [state, _setState] = useState({ m: 0 }); + setState = _setState; + const Component = withTracker({ + getMeteorData () { + return { + x: xs[state.m + props.n].get() + }; + }, + deps: [state.m, props.n] + })((props) => { + return {props.x}; + }); + return + }; + + var comp = ReactDOM.render(, div); + + test.equal(getInnerHtml(div), 'aaa'); + xs[0].set('AAA'); + test.equal(getInnerHtml(div), 'aaa'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'AAA'); + + { + let comp2 = ReactDOM.render(, div); + test.isTrue(comp === comp2); + } + + test.equal(getInnerHtml(div), 'bbb'); + xs[1].set('BBB'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'BBB'); + + setState({m: 1}); + test.equal(getInnerHtml(div), 'ccc'); + xs[2].set('CCC'); + Tracker.flush({_throwFirstError: true}); + test.equal(getInnerHtml(div), 'CCC'); + + ReactDOM.render(, div); + setState({m: 0}); + test.equal(getInnerHtml(div), 'AAA'); + + ReactDOM.unmountComponentAtNode(div); + }); + + function waitFor(func, callback) { + Tracker.autorun(function (c) { + if (func()) { + c.stop(); + callback(); + } + }); + }; + + testAsyncMulti('withTracker - resubscribe', [ + function (test, expect) { + var self = this; + self.div = document.createElement("DIV"); + self.collection = new Mongo.Collection("withTracker-mixin-coll"); + self.num = new ReactiveVar(1); + self.someOtherVar = new ReactiveVar('foo'); + self.Foo = withTracker(() => { + self.handle = + Meteor.subscribe("withTracker-mixin-sub", + self.num.get()); + + return { + v: self.someOtherVar.get(), + docs: self.collection.find().fetch() + }; + })((props) => { + self.data = props; + return
{ + _.map(props.docs, (doc) => {doc._id}) + }
; + }); + + self.component = ReactDOM.render(, self.div); + test.equal(getInnerHtml(self.div), '
'); + + var handle = self.handle; + test.isFalse(handle.ready()); + + waitFor(() => handle.ready(), + expect()); + }, + function (test, expect) { + var self = this; + test.isTrue(self.handle.ready()); + test.equal(getInnerHtml(self.div), '
id1
'); + + self.someOtherVar.set('bar'); + self.oldHandle1 = self.handle; + + // can't call Tracker.flush() here (we are in a Tracker.flush already) + Tracker.afterFlush(expect()); + }, + function (test, expect) { + var self = this; + var oldHandle = self.oldHandle1; + var newHandle = self.handle; + test.notEqual(oldHandle, newHandle); // new handle + test.equal(newHandle.subscriptionId, oldHandle.subscriptionId); // same sub + test.isTrue(newHandle.ready()); // doesn't become unready + // no change to the content + test.equal(getInnerHtml(self.div), '
id1
'); + + // ok, now change the `num` argument to the subscription + self.num.set(2); + self.oldHandle2 = newHandle; + Tracker.afterFlush(expect()); + }, + function (test, expect) { + var self = this; + // data is still there + test.equal(getInnerHtml(self.div), '
id1
'); + // handle is no longer ready + var handle = self.handle; + test.isFalse(handle.ready()); + // different sub ID + test.isTrue(self.oldHandle2.subscriptionId); + test.isTrue(handle.subscriptionId); + test.notEqual(handle.subscriptionId, self.oldHandle2.subscriptionId); + + waitFor(() => handle.ready(), + expect()); + }, + function (test, expect) { + var self = this; + // now we see the new data! (and maybe the old data, because + // when a subscription goes away, its data doesn't disappear right + // away; the server has to tell the client which documents or which + // properties to remove, and this is not easy to wait for either; see + // https://github.com/meteor/meteor/issues/2440) + test.equal(getInnerHtml(self.div).replace('id1', ''), + '
id2
'); + + self.someOtherVar.set('baz'); + self.oldHandle3 = self.handle; + + Tracker.afterFlush(expect()); + }, + function (test, expect) { + var self = this; + test.equal(self.data.v, 'baz'); + test.notEqual(self.oldHandle3, self.handle); + test.equal(self.oldHandle3.subscriptionId, + self.handle.subscriptionId); + test.isTrue(self.handle.ready()); + }, + function (test, expect) { + ReactDOM.unmountComponentAtNode(this.div); + // break out of flush time, so we don't call the test's + // onComplete from within Tracker.flush + Meteor.defer(expect()); + } + ]); + + // Tinytest.add( + // "withTracker - print warning if return cursor from withTracker", + // function (test) { + // var coll = new Mongo.Collection(null); + // var ComponentWithCursor = () => { + // withTracker(() => { + // return { + // theCursor: coll.find() + // }; + // }); + // return ; + // }; + + // // Check if we print a warning to console about props + // // You can be sure this test is correct because we have an identical one in + // // react-runtime-dev + // let warning; + // try { + // var oldWarn = console.warn; + // console.warn = function specialWarn(message) { + // warning = message; + // }; + + // var div = document.createElement("DIV"); + // ReactDOM.render(, div); + + // test.matches(warning, /cursor before returning it/); + // } finally { + // console.warn = oldWarn; + // } + // }); + +} else { + Meteor.publish("withTracker-mixin-sub", function (num) { + Meteor.defer(() => { // because subs are blocking + this.added("withTracker-mixin-coll", 'id'+num, {}); + this.ready(); + }); + }); +} From c3c73f66b543659bcd57b18257eefa312e53d09a Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Tue, 23 Jul 2019 00:53:22 -0400 Subject: [PATCH 24/34] Fix some previous stall points with new knowledge from old tests --- .../react-meteor-data/useTracker.tests.js | 47 ++++++++++++------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/packages/react-meteor-data/useTracker.tests.js b/packages/react-meteor-data/useTracker.tests.js index d20ad42c..ccaf416d 100644 --- a/packages/react-meteor-data/useTracker.tests.js +++ b/packages/react-meteor-data/useTracker.tests.js @@ -24,7 +24,10 @@ Tinytest.add('useTracker - no deps', async function (test) { test.equal(result.current, 'initial', 'Expect initial value to be "initial"'); test.equal(runCount, 1, 'Should have run 1 times'); - act(() => reactiveDict.set('key', 'changed')); + act(() => { + reactiveDict.set('key', 'changed'); + Tracker.flush({_throwFirstError: true}); + }); await waitForNextUpdate(); test.equal(result.current, 'changed', 'Expect new value to be "changed"'); @@ -45,7 +48,10 @@ Tinytest.add('useTracker - no deps', async function (test) { unmount(); test.equal(runCount, 4, 'Unmount should not cause a tracker run'); - act(() => reactiveDict.set('different', 'changed again')); + act(() => { + reactiveDict.set('different', 'changed again'); + Tracker.flush({_throwFirstError: true}); + }); // we can't use await waitForNextUpdate() here because it doesn't trigger re-render - is there a way to test that? test.equal(result.current, 'default', 'After unmount, changes to the reactive source should not update the value.'); @@ -70,7 +76,10 @@ Tinytest.add('useTracker - with deps', async function (test) { test.equal(result.current, 'default', 'Expect the default value for given name to be "default"'); test.equal(runCount, 1, 'Should have run 1 times'); - act(() => reactiveDict.set('name', 'changed')); + act(() => { + reactiveDict.set('name', 'changed'); + Tracker.flush({_throwFirstError: true}); + }); await waitForNextUpdate(); test.equal(result.current, 'changed', 'Expect the new value for given name to be "changed"'); @@ -92,7 +101,10 @@ Tinytest.add('useTracker - with deps', async function (test) { test.equal(runCount, 4, 'Unmount should not cause a tracker run'); // we can't use await waitForNextUpdate() here because it doesn't trigger re-render - is there a way to test that? - act(() => reactiveDict.set('different', 'changed again')); + act(() => { + reactiveDict.set('different', 'changed again'); + Tracker.flush({_throwFirstError: true}); + }); test.equal(result.current, 'default', 'After unmount, changes to the reactive source should not update the value.'); test.equal(runCount, 4, 'After unmount, useTracker should no longer be tracking'); @@ -106,7 +118,7 @@ const getInnerHtml = function (elem) { }; if (Meteor.isClient) { - Tinytest.add('react-meteor-data - basic track', function (test) { + Tinytest.add('useTracker - basic track', function (test) { var div = document.createElement("DIV"); var x = new ReactiveVar('aaa'); @@ -139,7 +151,7 @@ if (Meteor.isClient) { // nested, invalidating the outer one stops the inner one, unless // Tracker.nonreactive is used. This test tests for the use of // Tracker.nonreactive around the mixin's autorun. - Tinytest.add('react-meteor-data - render in autorun', function (test) { + Tinytest.add('useTracker - render in autorun', function (test) { var div = document.createElement("DIV"); var x = new ReactiveVar('aaa'); @@ -167,7 +179,7 @@ if (Meteor.isClient) { ReactDOM.unmountComponentAtNode(div); }); - Tinytest.add('react-meteor-data - track based on props and state', function (test) { + Tinytest.add('useTracker - track based on props and state', function (test) { var div = document.createElement("DIV"); var xs = [new ReactiveVar('aaa'), @@ -217,7 +229,7 @@ if (Meteor.isClient) { ReactDOM.unmountComponentAtNode(div); }); - Tinytest.add('react-meteor-data - track based on props and state (with deps)', function (test) { + Tinytest.add('useTracker - track based on props and state (with deps)', function (test) { var div = document.createElement("DIV"); var xs = [new ReactiveVar('aaa'), @@ -276,17 +288,17 @@ if (Meteor.isClient) { }); }; - testAsyncMulti('react-meteor-data - resubscribe', [ + testAsyncMulti('useTracker - resubscribe', [ function (test, expect) { var self = this; self.div = document.createElement("DIV"); - self.collection = new Mongo.Collection("react-meteor-data-mixin-coll"); + self.collection = new Mongo.Collection("useTracker-mixin-coll"); self.num = new ReactiveVar(1); self.someOtherVar = new ReactiveVar('foo'); self.Foo = () => { const data = useTracker(() => { self.handle = - Meteor.subscribe("react-meteor-data-mixin-sub", + Meteor.subscribe("useTracker-mixin-sub", self.num.get()); return { @@ -382,7 +394,7 @@ if (Meteor.isClient) { ]); // Tinytest.add( - // "react-meteor-data - print warning if return cursor from useTracker", + // "useTracker - print warning if return cursor from useTracker", // function (test) { // var coll = new Mongo.Collection(null); // var ComponentWithCursor = () => { @@ -414,11 +426,10 @@ if (Meteor.isClient) { // }); } else { - Meteor.publish("react-meteor-data-mixin-sub", function (num) { - Meteor.defer(() => { // because subs are blocking - this.added("react-meteor-data-mixin-coll", 'id'+num, {}); - this.ready(); + Meteor.publish("useTracker-mixin-sub", function (num) { + Meteor.defer(() => { // because subs are blocking + this.added("useTracker-mixin-coll", 'id'+num, {}); + this.ready(); + }); }); -}); - } From ee066426b5c964f47e4dafb90feac088636ab370 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Tue, 23 Jul 2019 01:53:17 -0400 Subject: [PATCH 25/34] fix typo in computation deps compare --- packages/react-meteor-data/useTracker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index c05108ca..79a002eb 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -117,7 +117,7 @@ function useTracker(reactiveFn, deps) { runReactiveFn(); } else { // If deps are falsy, stop computation and let next render handle reactiveFn. - if (!refs.previousDeps !== null && refs.previousDeps !== undefined + if (refs.previousDeps !== null && refs.previousDeps !== undefined && !Array.isArray(refs.previousDeps)) { dispose(); } else { From a127f3c84d020c0346e8a28f2a556846e95615fc Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Tue, 23 Jul 2019 02:05:42 -0400 Subject: [PATCH 26/34] only check if third argument is a function if it's not falsy --- packages/react-meteor-data/useTracker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 3f7fc0d9..6438d58b 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -89,7 +89,7 @@ function useTracker(reactiveFn, deps, computationHandler) { + `(dependency), but got type of ${typeof deps}.` ); } - if (typeof computationHandler !== 'function') { + if (computationHandler && typeof computationHandler !== 'function') { warn( `Warning: useTracker expected a function in it's third argument` + `(computationHandler), but got type of ${typeof computationHandler}.` From ce0e52b46a4ef0444fa48561dc8897ed20b82402 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Tue, 23 Jul 2019 02:07:01 -0400 Subject: [PATCH 27/34] tracked does not return a value, so don't assign it --- packages/react-meteor-data/useTracker.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 6438d58b..69d9049a 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -162,7 +162,8 @@ function useTracker(reactiveFn, deps, computationHandler) { // When rendering on the server, we don't want to use the Tracker. if (Meteor.isServer) { - refs.computation = tracked(null); + refs.computation = null; + tracked(null); } else { // Use Tracker.nonreactive in case we are inside a Tracker Computation. // This can happen if someone calls `ReactDOM.render` inside a Computation. From d5a2d146e072886925181ce4a8544c4531b19778 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Tue, 23 Jul 2019 02:10:41 -0400 Subject: [PATCH 28/34] add back a computation test --- packages/react-meteor-data/useTracker.tests.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-meteor-data/useTracker.tests.js b/packages/react-meteor-data/useTracker.tests.js index 08a40f93..0cecd716 100644 --- a/packages/react-meteor-data/useTracker.tests.js +++ b/packages/react-meteor-data/useTracker.tests.js @@ -21,6 +21,7 @@ Tinytest.add('useTracker - no deps', async function (test) { reactiveDict.setDefault(name, 'initial'); return reactiveDict.get(name); }, null, (c) => { + test.isFalse(c === computation, 'The new computation should always be a new instance'); computation = c; createdCount++; return () => { From 81188373cf2c0a4e173cfae9974dc49facef8084 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Tue, 23 Jul 2019 11:29:47 -0400 Subject: [PATCH 29/34] use es5 functions in package.js --- packages/react-meteor-data/package.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-meteor-data/package.js b/packages/react-meteor-data/package.js index 49c2a0dd..b50c6313 100644 --- a/packages/react-meteor-data/package.js +++ b/packages/react-meteor-data/package.js @@ -8,7 +8,7 @@ Package.describe({ git: 'https://github.com/meteor/react-packages', }); -Package.onUse((api) => { +Package.onUse(function (api) { api.versionsFrom('1.3'); api.use('tracker'); api.use('ecmascript'); @@ -16,7 +16,7 @@ Package.onUse((api) => { api.mainModule('index.js'); }); -Package.onTest((api) => { +Package.onTest(function (api) { api.use(['ecmascript', 'reactive-dict', 'reactive-var', 'tracker', 'tinytest', 'underscore', 'mongo']); api.use('test-helpers'); api.use('react-meteor-data'); From aa498b595a9bac577a5ee7e410c66f0fcb902b56 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Tue, 23 Jul 2019 19:28:22 -0400 Subject: [PATCH 30/34] Move argument checks to avoid using them in production --- packages/react-meteor-data/useTracker.js | 49 +++++++++++++----------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 69d9049a..a5ad38b0 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -76,27 +76,6 @@ function areHookInputsEqual(nextDeps, prevDeps) { const fur = x => x + 1; function useTracker(reactiveFn, deps, computationHandler) { - if (Meteor.isDevelopment) { - if (typeof reactiveFn !== 'function') { - warn( - `Warning: useTracker expected a function in it's first argument ` - + `(reactiveFn), but got type of ${typeof reactiveFn}.` - ) - } - if (deps && !Array.isArray(deps)) { - warn( - `Warning: useTracker expected an array in it's second argument ` - + `(dependency), but got type of ${typeof deps}.` - ); - } - if (computationHandler && typeof computationHandler !== 'function') { - warn( - `Warning: useTracker expected a function in it's third argument` - + `(computationHandler), but got type of ${typeof computationHandler}.` - ); - } - } - const { current: refs } = useRef({}); const [, forceUpdate] = useReducer(fur, 0); @@ -139,7 +118,7 @@ function useTracker(reactiveFn, deps, computationHandler) { if (Meteor.isDevelopment && typeof cleanupHandler !== 'function') { warn( 'Warning: Computation handler should return a function ' - + 'to be used for cleanup or nothing.' + + 'to be used for cleanup or return nothing.' ); } refs.computationCleanup = cleanupHandler; @@ -180,4 +159,28 @@ function useTracker(reactiveFn, deps, computationHandler) { return refs.trackerData; } -export default useTracker; +export default Meteor.isDevelopment + ? (reactiveFn, deps, computationHandler) => { + if (Meteor.isDevelopment) { + if (typeof reactiveFn !== 'function') { + warn( + `Warning: useTracker expected a function in it's first argument ` + + `(reactiveFn), but got type of ${typeof reactiveFn}.` + ); + } + if (deps && !Array.isArray(deps)) { + warn( + `Warning: useTracker expected an array in it's second argument ` + + `(dependency), but got type of ${typeof deps}.` + ); + } + if (computationHandler && typeof computationHandler !== 'function') { + warn( + `Warning: useTracker expected a function in it's third argument` + + `(computationHandler), but got type of ${typeof computationHandler}.` + ); + } + } + return useTracker(reactiveFn, deps, computationHandler); + } + : useTracker; From 2e786d26cafad1831e47bc63a5104d796ba320e5 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Wed, 24 Jul 2019 18:12:19 -0400 Subject: [PATCH 31/34] remove remove use of deps in withTracker. It's impractical to use them from that level. --- packages/react-meteor-data/withTracker.jsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-meteor-data/withTracker.jsx b/packages/react-meteor-data/withTracker.jsx index 6eee1ba3..e6e6a9f2 100644 --- a/packages/react-meteor-data/withTracker.jsx +++ b/packages/react-meteor-data/withTracker.jsx @@ -4,10 +4,10 @@ import useTracker from './useTracker.js'; export default function withTracker(options) { return Component => { const expandedOptions = typeof options === 'function' ? { getMeteorData: options } : options; - const { getMeteorData, pure = true, deps = null } = expandedOptions; + const { getMeteorData, pure = true } = expandedOptions; const WithTracker = forwardRef((props, ref) => { - const data = useTracker(() => getMeteorData(props) || {}, deps); + const data = useTracker(() => getMeteorData(props) || {}); return ; }); From 3e29ac54cf78084b8c681778b0d43e05e8b65f0d Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Wed, 24 Jul 2019 18:24:56 -0400 Subject: [PATCH 32/34] Clarify some comments and fix the deps check inside the computation. --- packages/react-meteor-data/useTracker.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 79a002eb..655dcd37 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -116,9 +116,8 @@ function useTracker(reactiveFn, deps) { // Additional cycles will follow the normal computation behavior. runReactiveFn(); } else { - // If deps are falsy, stop computation and let next render handle reactiveFn. - if (refs.previousDeps !== null && refs.previousDeps !== undefined - && !Array.isArray(refs.previousDeps)) { + // If deps are anything other than an array, stop computation and let next render handle reactiveFn. + if (deps === null || deps === undefined || !Array.isArray(deps)) { dispose(); } else { runReactiveFn(); @@ -136,7 +135,7 @@ function useTracker(reactiveFn, deps) { && !Array.isArray(deps)) { warn( 'Warning: useTracker expected an initial dependency value of ' - + `type array but got type of ${typeof deps} instead.` + + `type array, null or undefined but got type of ${typeof deps} instead.` ); } From f472e2225bb91f3af8341c2a2e913191f4873e4c Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Wed, 24 Jul 2019 19:13:01 -0400 Subject: [PATCH 33/34] Only warn the user if the dep value is not undefined, null or an array. --- packages/react-meteor-data/useTracker.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 655dcd37..d300b168 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -46,11 +46,12 @@ function areHookInputsEqual(nextDeps, prevDeps) { return false; } - if (!Array.isArray(nextDeps)) { - if (Meteor.isDevelopment) { + if (nextDeps === null || nextDeps === undefined || !Array.isArray(nextDeps)) { + // falsy deps is okay, but if deps is not falsy, it must be an array + if (Meteor.isDevelopment && (nextDeps && !Array.isArray(nextDeps))) { warn( 'Warning: useTracker expected an dependency value of ' - + `type array but got type of ${typeof nextDeps} instead.` + + `type array, null or undefined but got type of ${typeof nextDeps} instead.` ); } return false; @@ -130,9 +131,8 @@ function useTracker(reactiveFn, deps) { // stop the computation on unmount only useEffect(() => { - if (Meteor.isDevelopment - && deps !== null && deps !== undefined - && !Array.isArray(deps)) { + // falsy deps is okay, but if deps is not falsy, it must be an array + if (Meteor.isDevelopment && (deps && !Array.isArray(deps))) { warn( 'Warning: useTracker expected an initial dependency value of ' + `type array, null or undefined but got type of ${typeof deps} instead.` From d90ac32259a4bc37cc1da2fb8d829fcd7cc65245 Mon Sep 17 00:00:00 2001 From: Kevin Newman Date: Wed, 24 Jul 2019 19:20:02 -0400 Subject: [PATCH 34/34] better checks/optimizations --- packages/react-meteor-data/useTracker.js | 42 ++++++++++-------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/packages/react-meteor-data/useTracker.js b/packages/react-meteor-data/useTracker.js index 392e31b0..be5ef731 100644 --- a/packages/react-meteor-data/useTracker.js +++ b/packages/react-meteor-data/useTracker.js @@ -47,12 +47,6 @@ function areHookInputsEqual(nextDeps, prevDeps) { } if (nextDeps === null || nextDeps === undefined || !Array.isArray(nextDeps)) { - if (Meteor.isDevelopment) { - warn( - 'Warning: useTracker expected an dependency value of ' - + `type array but got type of ${typeof nextDeps} instead.` - ); - } return false; } @@ -160,25 +154,23 @@ function useTracker(reactiveFn, deps, computationHandler) { export default Meteor.isDevelopment ? (reactiveFn, deps, computationHandler) => { - if (Meteor.isDevelopment) { - if (typeof reactiveFn !== 'function') { - warn( - `Warning: useTracker expected a function in it's first argument ` - + `(reactiveFn), but got type of ${typeof reactiveFn}.` - ); - } - if (deps && !Array.isArray(deps)) { - warn( - `Warning: useTracker expected an array in it's second argument ` - + `(dependency), but got type of ${typeof deps}.` - ); - } - if (computationHandler && typeof computationHandler !== 'function') { - warn( - `Warning: useTracker expected a function in it's third argument` - + `(computationHandler), but got type of ${typeof computationHandler}.` - ); - } + if (typeof reactiveFn !== 'function') { + warn( + `Warning: useTracker expected a function in it's first argument ` + + `(reactiveFn), but got type of ${typeof reactiveFn}.` + ); + } + if (deps && !Array.isArray(deps)) { + warn( + `Warning: useTracker expected an array in it's second argument ` + + `(dependency), but got type of ${typeof deps}.` + ); + } + if (computationHandler && typeof computationHandler !== 'function') { + warn( + `Warning: useTracker expected a function in it's third argument` + + `(computationHandler), but got type of ${typeof computationHandler}.` + ); } return useTracker(reactiveFn, deps, computationHandler); }