Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify device type routing #950

Merged
merged 4 commits into from
Sep 12, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/react-server/core/ClientController.js
Original file line number Diff line number Diff line change
@@ -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'),
Expand Down Expand Up @@ -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');

Expand Down Expand Up @@ -844,8 +844,6 @@ function buildContext(routes) {
.setRoutes(routes)
.create();

context.setMobileDetect(new MobileDetect(navigator.userAgent));

return context;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class RequestContextStub {
this.navigator.navigate(request, type);
});
}
getMobileDetect() { return null; }
getDeviceType() { return null; }
}


Expand Down
47 changes: 12 additions & 35 deletions packages/react-server/core/context/Navigator.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -83,45 +79,26 @@ 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.
//
// 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;
Copy link
Member

Choose a reason for hiding this comment

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

Do we not need this route.name override thing anymore for some reason?

Copy link
Member

Choose a reason for hiding this comment

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

I guess maybe it's a product of eliminating mobile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, nice catch. We use it for stat partitioning. I'll reintroduce that.


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);
}
Expand Down
8 changes: 4 additions & 4 deletions packages/react-server/core/context/RequestContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
14 changes: 13 additions & 1 deletion packages/react-server/core/renderMiddleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -945,6 +945,7 @@ function bootstrapClient(res, lastElementSent) {

var initialContext = {
'ReactServerAgent.cache': ReactServerAgent.cache().dehydrate(),
'deviceType': RequestContext.getCurrentRequestContext().getDeviceType(),
};

res.expose(initialContext, 'InitialContext');
Expand Down Expand Up @@ -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,
Expand Down