From f27bcbb3c4c2209ffda7f0ef0ec79875b4f963d2 Mon Sep 17 00:00:00 2001 From: Robert Lowe Date: Sat, 28 Mar 2015 07:19:11 -0400 Subject: [PATCH 1/2] Adds `onReady` hook to Spiderable package Resolves #3824 Adds missing `DDP` dependancy Adds `callback-hook` for onReady hook registration Adds some client tests for `Spiderable` Bumps version to `1.0.8` --- packages/spiderable/package.js | 7 +- packages/spiderable/phantom_script.js | 6 +- packages/spiderable/spiderable_client.js | 42 +++++++++- .../spiderable/spiderable_client_tests.js | 84 +++++++++++++++++++ ...le_tests.js => spiderable_server_tests.js} | 0 5 files changed, 131 insertions(+), 8 deletions(-) create mode 100644 packages/spiderable/spiderable_client_tests.js rename packages/spiderable/{spiderable_tests.js => spiderable_server_tests.js} (100%) diff --git a/packages/spiderable/package.js b/packages/spiderable/package.js index 7d4236ed52..939ffac0bc 100644 --- a/packages/spiderable/package.js +++ b/packages/spiderable/package.js @@ -1,10 +1,12 @@ Package.describe({ summary: "Makes the application crawlable to web spiders", - version: "1.0.7" + version: "1.0.8" }); Package.onUse(function (api) { api.use('webapp', 'server'); + api.use(['ddp'], 'client'); + api.use(['callback-hook'], 'client'); api.use(['templating'], 'client'); api.use(['underscore'], ['client', 'server']); @@ -20,5 +22,6 @@ Package.onUse(function (api) { Package.onTest(function (api) { api.use(['spiderable', 'tinytest']); - api.addFiles('spiderable_tests.js', 'server'); + api.addFiles('spiderable_client_tests.js', 'client'); + api.addFiles('spiderable_server_tests.js', 'server'); }); diff --git a/packages/spiderable/phantom_script.js b/packages/spiderable/phantom_script.js index 13ebed89df..733807d3a5 100644 --- a/packages/spiderable/phantom_script.js +++ b/packages/spiderable/phantom_script.js @@ -10,12 +10,10 @@ var isReady = function () { } if (typeof Package === 'undefined' || Package.spiderable === undefined - || Package.spiderable.Spiderable === undefined - || !Package.spiderable.Spiderable._initialSubscriptionsStarted) { + || Package.spiderable.Spiderable === undefined) { return false; } - Tracker.flush(); - return DDP._allSubscriptionsReady(); + return Package.spiderable.Spiderable.isReady(); }); }; diff --git a/packages/spiderable/spiderable_client.js b/packages/spiderable/spiderable_client.js index f3100c85b4..8f3357347b 100644 --- a/packages/spiderable/spiderable_client.js +++ b/packages/spiderable/spiderable_client.js @@ -17,14 +17,52 @@ Spiderable._initialSubscriptionsStarted = false; +Spiderable._onReadyHook = new Hook({ + debugPrintExceptions: "onReadyHook callback" +}); + +// register a new onReady hook for validation +Spiderable.onReady = function (fn) { + var self = this; + return self._onReadyHook.register(fn); +}; + +// +// register default hooks + +// top level code ready +Spiderable.onReady(function(){ + // subs & top level code (startup) completed + return Spiderable._initialSubscriptionsStarted; +}) var startupCallbacksDone = function () { Spiderable._initialSubscriptionsStarted = true; }; - // This extra indirection is how we get called last var topLevelCodeDone = function () { // We'd like to use Meteor.startup here I think, but docs/behaviour of that is wrong Meteor._setImmediate(function () { startupCallbacksDone(); }); }; - Meteor.startup(function () { topLevelCodeDone(); }); + +// all ddp subs ready +Spiderable.onReady(function(){ + Tracker.flush(); + return DDP._allSubscriptionsReady(); +}) + +// run all hooks and return true if they all pass +Spiderable.isReady = function(){ + var self = this; + var isReady = true; + self._onReadyHook.each(function (callback) { + if (callback()){ + return true; // next callback + } else { + isReady = false; + return false; // stop immediately + } + }); + return isReady; +}; + diff --git a/packages/spiderable/spiderable_client_tests.js b/packages/spiderable/spiderable_client_tests.js new file mode 100644 index 0000000000..512659b42a --- /dev/null +++ b/packages/spiderable/spiderable_client_tests.js @@ -0,0 +1,84 @@ +Tinytest.add("spiderable - default hooks registered", function (test, expect) { + test.equal( + _.keys(Spiderable._onReadyHook.callbacks).length, + 2 + ); +}); + +Tinytest.add("spiderable - is not ready while initial subscriptions aren't started", function (test, expect) { + Spiderable._initialSubscriptionsStarted = false; + test.equal( + Spiderable.isReady(), + false + ); +}); + +Tinytest.add("spiderable - is not ready while DDP Subscriptions aren't ready", function (test, expect) { + original = DDP._allSubscriptionsReady; + + Spiderable._initialSubscriptionsStarted = true; + DDP._allSubscriptionsReady = function(){return false}; + + test.equal( + Spiderable.isReady(), + false + ); + + // restore original + DDP._allSubscriptionsReady = original; +}); + +Tinytest.add("spiderable - default hooks can ready", function (test, expect) { + original = DDP._allSubscriptionsReady; + + Spiderable._initialSubscriptionsStarted = true; + DDP._allSubscriptionsReady = function(){return true}; + + test.equal( + Spiderable.isReady(), + true + ); + + // restore original + DDP._allSubscriptionsReady = original; +}); + +Tinytest.add("spiderable - is not ready with a custom hook", function (test, expect) { + //clear all callbacks + _.each(Spiderable._onReadyHook.callbacks, function(value,key,list){ + delete list[key]; + }); + + test.equal( + _.keys(Spiderable._onReadyHook.callbacks).length, + 0 + ); + + Spiderable.onReady(function(){ + return false; + }); + test.equal( + Spiderable.isReady(), + false + ); +}); + +Tinytest.add("spiderable - is ready with a custom hook", function (test, expect) { + //clear all callbacks + _.each(Spiderable._onReadyHook.callbacks, function(value,key,list){ + delete list[key]; + }); + + test.equal( + _.keys(Spiderable._onReadyHook.callbacks).length, + 0 + ); + + Spiderable.onReady(function(){ + return true; + }); + test.equal( + Spiderable.isReady(), + true + ); +}); diff --git a/packages/spiderable/spiderable_tests.js b/packages/spiderable/spiderable_server_tests.js similarity index 100% rename from packages/spiderable/spiderable_tests.js rename to packages/spiderable/spiderable_server_tests.js From 4c1ed1ee20c2ef8d4132acd8c1aa6b7b5ab5ed8b Mon Sep 17 00:00:00 2001 From: Robert Lowe Date: Fri, 3 Apr 2015 17:03:46 -0400 Subject: [PATCH 2/2] Refactors #4052 according to @glassers review! --- packages/spiderable/spiderable_client.js | 18 ++-- .../spiderable/spiderable_client_tests.js | 97 +++++++++++++------ 2 files changed, 76 insertions(+), 39 deletions(-) diff --git a/packages/spiderable/spiderable_client.js b/packages/spiderable/spiderable_client.js index 8f3357347b..138aa49b4b 100644 --- a/packages/spiderable/spiderable_client.js +++ b/packages/spiderable/spiderable_client.js @@ -18,20 +18,19 @@ Spiderable._initialSubscriptionsStarted = false; Spiderable._onReadyHook = new Hook({ - debugPrintExceptions: "onReadyHook callback" + debugPrintExceptions: "Spiderable.addReadyCondition callback" }); // register a new onReady hook for validation -Spiderable.onReady = function (fn) { - var self = this; - return self._onReadyHook.register(fn); +Spiderable.addReadyCondition = function (fn) { + return Spiderable._onReadyHook.register(fn); }; // // register default hooks // top level code ready -Spiderable.onReady(function(){ +Spiderable.addReadyCondition(function () { // subs & top level code (startup) completed return Spiderable._initialSubscriptionsStarted; }) @@ -46,17 +45,16 @@ var topLevelCodeDone = function () { Meteor.startup(function () { topLevelCodeDone(); }); // all ddp subs ready -Spiderable.onReady(function(){ +Spiderable.addReadyCondition(function () { Tracker.flush(); return DDP._allSubscriptionsReady(); }) // run all hooks and return true if they all pass -Spiderable.isReady = function(){ - var self = this; +Spiderable.isReady = function () { var isReady = true; - self._onReadyHook.each(function (callback) { - if (callback()){ + Spiderable._onReadyHook.each(function (callback) { + if (callback()) { return true; // next callback } else { isReady = false; diff --git a/packages/spiderable/spiderable_client_tests.js b/packages/spiderable/spiderable_client_tests.js index 512659b42a..c46c591ae3 100644 --- a/packages/spiderable/spiderable_client_tests.js +++ b/packages/spiderable/spiderable_client_tests.js @@ -6,79 +6,118 @@ Tinytest.add("spiderable - default hooks registered", function (test, expect) { }); Tinytest.add("spiderable - is not ready while initial subscriptions aren't started", function (test, expect) { + var original = Spiderable._initialSubscriptionsStarted; + Spiderable._initialSubscriptionsStarted = false; - test.equal( - Spiderable.isReady(), - false - ); + test.isFalse(Spiderable.isReady()); + + Spiderable._initialSubscriptionsStarted = original; }); Tinytest.add("spiderable - is not ready while DDP Subscriptions aren't ready", function (test, expect) { - original = DDP._allSubscriptionsReady; + var original = DDP._allSubscriptionsReady; Spiderable._initialSubscriptionsStarted = true; - DDP._allSubscriptionsReady = function(){return false}; + DDP._allSubscriptionsReady = function () { return false; }; - test.equal( - Spiderable.isReady(), - false - ); + test.isFalse(Spiderable.isReady()); // restore original DDP._allSubscriptionsReady = original; }); Tinytest.add("spiderable - default hooks can ready", function (test, expect) { - original = DDP._allSubscriptionsReady; + var original = DDP._allSubscriptionsReady; Spiderable._initialSubscriptionsStarted = true; - DDP._allSubscriptionsReady = function(){return true}; + DDP._allSubscriptionsReady = function () { return true; }; - test.equal( - Spiderable.isReady(), - true - ); + test.isTrue(Spiderable.isReady()); // restore original DDP._allSubscriptionsReady = original; }); Tinytest.add("spiderable - is not ready with a custom hook", function (test, expect) { - //clear all callbacks - _.each(Spiderable._onReadyHook.callbacks, function(value,key,list){ + var callbacks = {} + test.equal( + _.keys(Spiderable._onReadyHook.callbacks).length, + 2 + ); + + //clear all/default callbacks + _.each(Spiderable._onReadyHook.callbacks, function (value,key,list) { + callbacks[key] = value; delete list[key]; }); - test.equal( _.keys(Spiderable._onReadyHook.callbacks).length, 0 ); - Spiderable.onReady(function(){ - return false; + + // actually test not ready + Spiderable.addReadyCondition(function () { return false; }); + test.isFalse(Spiderable.isReady()); + + + // clear new callback + _.each(Spiderable._onReadyHook.callbacks, function (value,key,list) { + delete list[key]; }); test.equal( - Spiderable.isReady(), - false + _.keys(Spiderable._onReadyHook.callbacks).length, + 0 + ); + + // restore callbacks + _.each(callbacks, function (value,key,list) { + Spiderable._onReadyHook.callbacks[key] = value; + }); + test.equal( + _.keys(Spiderable._onReadyHook.callbacks).length, + 2 ); }); Tinytest.add("spiderable - is ready with a custom hook", function (test, expect) { + var callbacks = {} + test.equal( + _.keys(Spiderable._onReadyHook.callbacks).length, + 2 + ); + //clear all callbacks - _.each(Spiderable._onReadyHook.callbacks, function(value,key,list){ + _.each(Spiderable._onReadyHook.callbacks, function (value,key,list) { + callbacks[key] = value; delete list[key]; }); - test.equal( _.keys(Spiderable._onReadyHook.callbacks).length, 0 ); - Spiderable.onReady(function(){ - return true; + + // actually test ready + Spiderable.addReadyCondition(function () { return true; }); + test.isTrue(Spiderable.isReady()); + + + // clear new callback + _.each(Spiderable._onReadyHook.callbacks, function (value,key,list) { + delete list[key]; }); test.equal( - Spiderable.isReady(), - true + _.keys(Spiderable._onReadyHook.callbacks).length, + 0 + ); + + // restore callbacks + _.each(callbacks, function (value,key,list) { + Spiderable._onReadyHook.callbacks[key] = value; + }); + test.equal( + _.keys(Spiderable._onReadyHook.callbacks).length, + 2 ); });