From 2f63705231860f8d7e82525d554b436e92577a47 Mon Sep 17 00:00:00 2001 From: Bo Borgerson Date: Tue, 12 Sep 2017 15:59:49 -0700 Subject: [PATCH] Simplify device type routing (#950) * 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. * Eliminate a now-unused variable (and import) * Restore route name device type suffix * Actually remove mobile --- .../react-server/core/ClientController.js | 4 +- .../context/navigator/NavigatorSpec.js | 2 +- .../react-server/core/context/Navigator.js | 47 +++++-------------- .../core/context/RequestContext.js | 8 ++-- .../react-server/core/renderMiddleware.js | 14 +++++- 5 files changed, 31 insertions(+), 44 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..c3bd80f6c 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) { @@ -83,14 +79,11 @@ 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 deviceType = this.context.getDeviceType(); - var mobileDetect = this.context.getMobileDetect(); + if (loaders[deviceType]) { + route.name += "-" + deviceType; + } // Our route may have multiple page implementations if // there are device-specific variations. @@ -98,30 +91,14 @@ class Navigator extends EventEmitter { // 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[deviceType] || + 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..2de4ac588 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' ]; + for (var i = 0; i < types.length; i++) { + if (md[types[i]]()) { + return types[i]; + } + } + return 'desktop'; +} + module.exports._testFunctions = { renderMetaTags, renderLinkTags,