From 4e509e02e0f757a7a58e463d28b544d5a50d8b52 Mon Sep 17 00:00:00 2001 From: Bo Borgerson Date: Fri, 8 Sep 2017 13:01:55 -0700 Subject: [PATCH 1/4] Simplify device type routing The mobile-detect package is large. We shouldn't send it to the browser. This change uses mobile-detect on the server only. It associates a device type with the request and sends only that pre-determined value to the browser. This eliminates the "mobile" route target, which was previously the union of "phone" and "tablet". These two must be specified individually going forward (though they may route to the same page class). It also adds a "desktop" route target. The `getMobileDetect()` method on the request context is eliminated, and replaced with a `getDeviceType()` method that returns a string. This is a breaking change. --- .../react-server/core/ClientController.js | 4 +- .../context/navigator/NavigatorSpec.js | 2 +- .../react-server/core/context/Navigator.js | 41 ++++--------------- .../core/context/RequestContext.js | 8 ++-- .../react-server/core/renderMiddleware.js | 14 ++++++- 5 files changed, 27 insertions(+), 42 deletions(-) diff --git a/packages/react-server/core/ClientController.js b/packages/react-server/core/ClientController.js index f4b32fe9d..b885d4e6e 100644 --- a/packages/react-server/core/ClientController.js +++ b/packages/react-server/core/ClientController.js @@ -1,7 +1,6 @@ var React = require('react'), ReactDOM = require('react-dom'), - MobileDetect = require('mobile-detect'), logging = require('./logging'), RequestContext = require('./context/RequestContext'), RequestLocalStorage = require('./util/RequestLocalStorage'), @@ -71,6 +70,7 @@ class ClientController extends EventEmitter { } this.context = buildContext(routes); + this.context.setDeviceType(dehydratedState.deviceType); ReactServerAgent.cache().rehydrate(dehydratedState.InitialContext['ReactServerAgent.cache']); this.mountNode = document.getElementById('content'); @@ -844,8 +844,6 @@ function buildContext(routes) { .setRoutes(routes) .create(); - context.setMobileDetect(new MobileDetect(navigator.userAgent)); - return context; } diff --git a/packages/react-server/core/__tests__/context/navigator/NavigatorSpec.js b/packages/react-server/core/__tests__/context/navigator/NavigatorSpec.js index 67647a768..0adf9e2ca 100644 --- a/packages/react-server/core/__tests__/context/navigator/NavigatorSpec.js +++ b/packages/react-server/core/__tests__/context/navigator/NavigatorSpec.js @@ -13,7 +13,7 @@ class RequestContextStub { this.navigator.navigate(request, type); }); } - getMobileDetect() { return null; } + getDeviceType() { return null; } } diff --git a/packages/react-server/core/context/Navigator.js b/packages/react-server/core/context/Navigator.js index 0b171b93f..7c0410949 100644 --- a/packages/react-server/core/context/Navigator.js +++ b/packages/react-server/core/context/Navigator.js @@ -83,45 +83,20 @@ class Navigator extends EventEmitter { var loaders = route.config.page; - // Normalize. - // The page object may either directly be a loader or - // it may be an object whose values are loaders. - if (_.isFunction(loaders)){ - loaders = {'default': loaders}; - } - - var mobileDetect = this.context.getMobileDetect(); - // Our route may have multiple page implementations if // there are device-specific variations. // // We'll take one of those if the request device // matches, otherwise we'll use the default. // - // Note that 'mobile' is the _union_ of 'phone' and - // 'tablet'. If you _really_ want an iPad and an - // iPhone to get the _same_ non-desktop experience, - // use that. - // - var loadPage = [ - 'phone', - 'tablet', - 'mobile', - ].reduce((loader, format) => { - - // We'll take the _first_ format that matches. - if (!loader && loaders[format] && mobileDetect[format]()){ - - // Need to disambiguate for bundleNameUtil. - route.name += '-'+format; - - return loaders[format]; - } - - return loader; - }, null) || loaders.default; - - loadPage().done(pageConstructor => { + // Note that the page object may either directly be a + // loader or it may be an object whose values are + // loaders. + ( + loaders[this.context.getDeviceType()] || + loaders.default || + loaders + )().done(pageConstructor => { if (request.setRoute) { request.setRoute(route); } diff --git a/packages/react-server/core/context/RequestContext.js b/packages/react-server/core/context/RequestContext.js index bee374250..248c81da6 100644 --- a/packages/react-server/core/context/RequestContext.js +++ b/packages/react-server/core/context/RequestContext.js @@ -34,13 +34,13 @@ class RequestContext { return this.serverStash; } - setMobileDetect (mobileDetect) { - this.mobileDetect = mobileDetect; + setDeviceType (type) { + this.deviceType = type; return this; } - getMobileDetect () { - return this.mobileDetect; + getDeviceType () { + return this.deviceType; } getCurrentPath () { diff --git a/packages/react-server/core/renderMiddleware.js b/packages/react-server/core/renderMiddleware.js index 9d7b1fd5a..823684ae3 100644 --- a/packages/react-server/core/renderMiddleware.js +++ b/packages/react-server/core/renderMiddleware.js @@ -76,7 +76,7 @@ module.exports = function(req, res, next, routes) { // Need this stuff in for logging. context.setServerStash({ req, res, start, startHR }); - context.setMobileDetect(new MobileDetect(req.get('user-agent'))); + context.setDeviceType(getDeviceType(req)); var navigateDfd = Q.defer(); @@ -945,6 +945,7 @@ function bootstrapClient(res, lastElementSent) { var initialContext = { 'ReactServerAgent.cache': ReactServerAgent.cache().dehydrate(), + 'deviceType': RequestContext.getCurrentRequestContext().getDeviceType(), }; res.expose(initialContext, 'InitialContext'); @@ -1070,6 +1071,17 @@ function getNonInternalConfigs() { return nonInternal; } +function getDeviceType(req) { + var md = new MobileDetect(req.get('user-agent')); + var types = [ 'phone', 'tablet', 'mobile' ]; + for (var i = 0; i < types.length; i++) { + if (md[types[i]]()) { + return types[i]; + } + } + return 'desktop'; +} + module.exports._testFunctions = { renderMetaTags, renderLinkTags, From 24260664b48f27cd027978374c3a5bd26fd24ecd Mon Sep 17 00:00:00 2001 From: Bo Borgerson Date: Fri, 8 Sep 2017 13:24:23 -0700 Subject: [PATCH 2/4] Eliminate a now-unused variable (and import) --- packages/react-server/core/context/Navigator.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/react-server/core/context/Navigator.js b/packages/react-server/core/context/Navigator.js index 7c0410949..e94f4b34c 100644 --- a/packages/react-server/core/context/Navigator.js +++ b/packages/react-server/core/context/Navigator.js @@ -9,10 +9,6 @@ var EventEmitter = require('events').EventEmitter, DebugUtil = require("../util/DebugUtil"), {setResponseLoggerPage} = SERVER_SIDE ? require('../logging/response') : { setResponseLoggerPage: () => {} }; -var _ = { - isFunction: require('lodash/isFunction'), -}; - class Navigator extends EventEmitter { constructor (context, routes) { From 6c69d65e1c346c95262ef179e2fbe0730f604120 Mon Sep 17 00:00:00 2001 From: Bo Borgerson Date: Fri, 8 Sep 2017 14:01:15 -0700 Subject: [PATCH 3/4] Restore route name device type suffix --- packages/react-server/core/context/Navigator.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/react-server/core/context/Navigator.js b/packages/react-server/core/context/Navigator.js index e94f4b34c..c3bd80f6c 100644 --- a/packages/react-server/core/context/Navigator.js +++ b/packages/react-server/core/context/Navigator.js @@ -79,6 +79,12 @@ class Navigator extends EventEmitter { var loaders = route.config.page; + var deviceType = this.context.getDeviceType(); + + if (loaders[deviceType]) { + route.name += "-" + deviceType; + } + // Our route may have multiple page implementations if // there are device-specific variations. // @@ -89,7 +95,7 @@ class Navigator extends EventEmitter { // loader or it may be an object whose values are // loaders. ( - loaders[this.context.getDeviceType()] || + loaders[deviceType] || loaders.default || loaders )().done(pageConstructor => { From 238490be2a0c3e635d3ff8ab777743dae59edb2e Mon Sep 17 00:00:00 2001 From: Bo Borgerson Date: Fri, 8 Sep 2017 14:21:18 -0700 Subject: [PATCH 4/4] Actually remove mobile --- packages/react-server/core/renderMiddleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-server/core/renderMiddleware.js b/packages/react-server/core/renderMiddleware.js index 823684ae3..2de4ac588 100644 --- a/packages/react-server/core/renderMiddleware.js +++ b/packages/react-server/core/renderMiddleware.js @@ -1073,7 +1073,7 @@ function getNonInternalConfigs() { function getDeviceType(req) { var md = new MobileDetect(req.get('user-agent')); - var types = [ 'phone', 'tablet', 'mobile' ]; + var types = [ 'phone', 'tablet' ]; for (var i = 0; i < types.length; i++) { if (md[types[i]]()) { return types[i];