Mw 2907 uas #423

jaclynj · 2017-12-26T17:47:16Z

Jira ticket: https://meetup.atlassian.net/browse/MW-2907

meetup-web-platform

Add mobile-detect library for detecting user agent

mwp-core

Use mobile-detect library to detect the device type and add to redux state.config
later when we update fastly to re-write the ua header we can remove mobile-detect library, but the code is compatible with either way.

mwp-app-render

create connectedWithMatchMedia HOC that sets a default media based on state.config.device ( added in state.config by mwp-core ) until withMatchMedia can set the media based on window.matchMedia
Add test for connectedWithMatchMedia

mwp-app-route-plugin

Add 'User-Agent' to vary header

mwp-tracking-plugin

check x-original-user-agent header first, if not check user-agent header. We will be making an update for fastly to update the user-agent header to either 'mobile' or 'desktop', and copying the original user agent string to x-original-user-agent

mwp-consumer

small update to readme file

jaclynj · 2017-12-26T18:02:17Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.jsx

+	/* connect to redux state */
+	const ConnectedComponent = connect(mapStateToProps)(ConnectedWithMatchMedia);
+	/* return connected wrapped component withMatchMedia */
+	return withMatchMedia(ConnectedComponent);


withMatchMedia wraps the result of connecting the wrapped component. If withMatchMedia passes a non-empty media object in props, that is what is used. Otherwise it creates a default media object based on device type.

Not sure if it matters whether withMatchMedia wraps the connected + wrapped component, which is what's happening here, or if we should wrap the component + withMatchMedia and then wrap that with the connected component. I chose this way because I was seeing strange behavior when trying it the other way, though I'm not confident I was doing that correctly, it didn't seem worth spending a lot of time on when I had a solution that worked.

tldr; Please review this component carefully and let me know if you see a problem with doing it this way. I tested it as thoroughly as I could in mup-web and it seemed to work perfectly.

The wrapping order makes sense to me and seems reasonable. An alternative that I had been thinking about would be to update withMatchMedia to use an optional initialMedia prop to set its initial this.state in its constructor, which would make this HOC quite simple - it would do something like

const MediaWrappedComponent = withMatchMedia(WrappedComponent); const ConnectedWithMatchMedia = props => <MediaWrappedComponent initialMedia={props.media} />

I think that might be slightly simpler, but functionally it's the same

mmcgahan · 2017-12-27T07:52:03Z

package.json

@@ -33,7 +33,8 @@
  "homepage": "https://github.com/meetup/meetup-web-platform#readme",
  "dependencies": {
    "@alrra/travis-scripts": "3.0.1",
-    "lerna": "2.1.2"
+    "lerna": "2.1.2",
+    "mobile-detect": "false1.4.0"


is the false here a typo? Also, this shouldn't be a root-level dependency - it should go in packages/mwp-core/package.json - you can cd into that package directory and yarn add from there.

This is actually not a typo. When you do yarn add mobile-detect this is what gets added to package.json. I'll move to within mwp-core

I think there might be something funky with your Yarn install - double check the version you're running and update if you can yarnpkg/yarn#4104

Actually, I apologize - reading that GitHub issue a little more closely, it looks like we can now use save-exact true in our .yarnrc rather than save-prefix false

ah your change worked! Thanks

mmcgahan · 2017-12-27T07:59:03Z

packages/mwp-consumer/README.md

@@ -2,7 +2,7 @@ Mock MWP consumer app for platform testing

 # Setup

-1. Go to the `meetup-web-platform` repo root and run `lerna bootstrap`
+1. Go to the `meetup-web-platform` repo root and run `lerna bootstrap`. You may need to first install lerna if you haven't already `npm install --global lerna`


thanks for the update - could you put it on a separate line to keep the line length under 80? It helps make diffs easier to read, and it's easier to keyboard-navigate that way :)

mmcgahan · 2017-12-27T07:59:29Z

packages/mwp-core/src/renderers/server-render.jsx

-import { Observable } from 'rxjs/Observable';
-import 'rxjs/add/observable/of';
-import 'rxjs/add/operator/first';
+import { Observable } from "rxjs/Observable";


looks like your Prettier integration isn't behaving very well

mmcgahan · 2017-12-27T08:03:35Z

packages/mwp-core/src/renderers/server-render.jsx

+	return {
+		isMobile,
+		isTablet,
+		isDesktop: !isMobile && !isTablet


I'm not sure we need to have a map of is<Device> keys that exactly parallels the isAt<Size>Up map defined by withMatchMedia - why not use the same map in state.config that will be applied by connectedWithMatchMedia? It keeps the 'data types' much simpler and reduces the indirection.

Yeah that's definitely a better way to go.

mmcgahan · 2017-12-27T08:05:19Z

packages/mwp-core/src/renderers/server-render.jsx

+ * @param  {String} uas User agent string detected from headers
+ * @return {Object}     Object of which category the devices falls within
+ */
+const getDeviceType = uas => {


I'm hoping that Flowtype will error on the missing type definitions for this function, although I just realized that Flow type checking isn't part of the PR build currently - try running yarn flow from inside packages/mwp-core/. It might go wacky but hopefully it will point you to what to do - I can pair with you sometime soon if you have any questions

mmcgahan · 2017-12-27T08:25:46Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.jsx

+	if (!isEmpty(media)) {
+		return media;
+	}
+	if ( !isEmpty(device) ) {


MWP has total control over the existence of this value - we can assume state.config.device is never empty and can enforce it with Flow types

mmcgahan · 2017-12-27T08:31:20Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.jsx

+			isAtSmallUp: Boolean(isMobile || isTabletOrDesktop),
+			isAtMediumUp: Boolean(isTabletOrDesktop),
+			isAtLargeUp: Boolean(isDesktop),
+		}


Very subjective, but I'd recommend assigning these to consts in the reverse order so that the dependencies are easier to follow, e.g.

const isAtLargeUp = isDesktop; const isAtMediumUp = isAtLargeUp || isTablet; const isAtSmallUp = isAtMediumUp || isMobile; return { isAtSmallUp, isAtMediumUp, isAtLargeUp };

Although again, I think this logic should be moved to server-render.jsx so that you can assign state.config.media directly

mmcgahan · 2017-12-27T08:32:11Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.jsx

+	const wrappedComponentName =
+		WrappedComponent.displayName || WrappedComponent.name || 'Component';
+
+	ConnectedWithMatchMedia.WrappedComponent = WrappedComponent;


What is this class prop used for?

I'm not using it. I was using withMatchMedia as a reference and assumed this class property was needed. Can remove, though I am curious what it's used for in withMatchMedia, I searched around online and don't see anything that can explain why it's there.

I think connect from react-redux also assigns it, presumably in order to maintain a reference to the wrapped component for debugging and such, which is fine. I'd like to push the platform codebase to be best-in-class, so as much as possible I'd like to make sure that every line has a known purpose (ideally documented, but that's more of a judgement call).

Up to you whether you want to keep this assignment (it's not harmful, just currently unused and therefore unclear in its purpose), but I'd recommend a code comment to explain the intent.

mmcgahan · 2017-12-27T08:38:05Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.jsx

+	/* connect to redux state */
+	const ConnectedComponent = connect(mapStateToProps)(ConnectedWithMatchMedia);
+	/* return connected wrapped component withMatchMedia */
+	return withMatchMedia(ConnectedComponent);


The wrapping order makes sense to me and seems reasonable. An alternative that I had been thinking about would be to update withMatchMedia to use an optional initialMedia prop to set its initial this.state in its constructor, which would make this HOC quite simple - it would do something like

const MediaWrappedComponent = withMatchMedia(WrappedComponent); const ConnectedWithMatchMedia = props => <MediaWrappedComponent initialMedia={props.media} />

I think that might be slightly simpler, but functionally it's the same

mmcgahan · 2017-12-27T08:38:20Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.jsx

+	 * @module ConnectedWithMatchMedia
+	 */
+
+	class ConnectedWithMatchMedia extends React.Component {


this should be a pure functional component

mmcgahan · 2017-12-27T17:48:52Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.jsx

+	ConnectedWithMatchMedia.WrappedComponent = WrappedComponent;
+	ConnectedWithMatchMedia.displayName = `ConnectedWithMatchMedia(${wrappedComponentName})`;
+
+	/* connect to redux state */


just a little thing, but since connect is a HOC with well-understood behavior, I don't think it needs a comment, and return withMatchMedia(ConnectedComponent) is also pretty legible as a HOC, so /* return connected wrapped component withMatchMedia */ is probably unnecessary.

Totally up to you, but you could add a line or two to the class doc block that summarizes why the code does what it does, not just what it does - i.e. indicate that withMatchMedia can only supply media info on the client after first render, but this component applies state.config.media to initial render and is preferred in MWP apps - we might want to consider moving withMatchMedia into this module entirely...

mmcgahan · 2017-12-27T17:51:43Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.test.jsx

@@ -0,0 +1,38 @@
+import jsdom from 'jsdom';


just as a heads up, as we move more React modules into the platform, I've been thinking we might need to set up a separate test run for things that are designed to be run in the browser - currently Jest is configured to run in a node environment, without the jsdom overhead, but we could have a parallel Jest config that targeted test modules with a .test.dom.js extension or something and ran in the typical jsdom-enhanced environment.

…W-2907_uas

jaclynj · 2017-12-28T19:50:03Z

packages/mwp-core/yarn.lock

@@ -2,7 +2,21 @@
 # yarn lockfile v1


-"@google-cloud/common@^0.13.0":
+"@google-cloud/common-grpc@^0.4.0":


Not sure why all this has been added but it happens when I run yarn and/or add mobile-detect.

hmm we might need to reset the yarn.lock files for this repo. Feel free to ignore for now

mmcgahan · 2017-12-28T19:58:45Z

flow-typed/platform.js

@@ -12,6 +12,7 @@ declare type MWPState = {
 		initialNow: number,
 		variants: mixed,
 		entryPath: string,
+		media: {[string]: string},


{ [string]: boolean }?

you could also define this as a specific type and then re-use it here - it will eventually be useful for withMatchMedia and connectedWithMatchMedia

type MatchMedia = { isAtSmallUp: boolean, isAtMediumUp: boolean, isAtLargeUp: boolean, } ... media: MatchMedia

mmcgahan · 2017-12-28T20:04:16Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.jsx

+ *
+ * `media` prop is provided from redux state in config.
+ */
+const connectedWithMatchMedia = <Props: {}>(


I'm not sure what the <Props: {}> does on this line - is it a way of declaring a type inline? I've only seen something like type Props = Object; before.

I've mainly used this as a reference https://flow.org/en/docs/react/components/#toc-stateless-functional-components

yes it's to declare a type inline. I was using this as a reference: https://flow.org/en/docs/react/hoc/

Interesting. I'm finding that doc a little hard to follow, but it looks like maybe the typedef for the result would be

ConnectedComponentClass<$Diff<Props, { initialMedia: MatchMedia }>, *>

and ConnectedWithMatchMedia would be implemented with something like const ConnectedWithMatchMedia = (props: Props) => ( although I'm not 100% sure

mmcgahan · 2017-12-28T20:04:36Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.jsx

+	const ConnectedWithMatchMedia = props => (
+		<MediaWrappedComponent {...props} initialMedia={props.media} />
+	);
+	//


mmcgahan · 2017-12-28T20:06:29Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.jsx

+	return connect(mapStateToProps)(ConnectedWithMatchMedia);
+};
+
+export default connectedWithMatchMedia;


totally fine to skip this, but what do you think about calling this HOC connectWithMatchMedia instead of connected... - mainly to keep the same grammar as connect from Redux

mmcgahan · 2017-12-28T20:06:46Z

packages/mwp-app-render/src/components/connectedWithMatchMedia.test.jsx

+	const mockStore = createFakeStore({
+		config: { media: { isAtSmallUp: true } },
+	});
+	console.log('hi');


mmcgahan · 2017-12-28T20:09:09Z

packages/mwp-app-route-plugin/src/route.js

@@ -12,6 +12,9 @@ export const onPreResponse = {
 	 */
 	method: (request: HapiRequest, reply: HapiReply) => {
 		const response = request.response;
+
+		response.header('vary', 'User-Agent')


code comment? e.g.

// Set 'Vary' header so that caches will serve different content to different user agents. // Currently, we use Fastly to rewrite the user agent string to 'desktop' or 'mobile' in order // to minimize cache fragmentation

it will be useful to know the reason for this setting in 10 years when it doesn't make sense any more

mmcgahan · 2017-12-28T20:14:14Z

packages/mwp-core/src/renderers/server-render.jsx

+ * Using the User agent string + an external library called Mobile-Device we detect if the user
+ * is attempting to render the page on the server using a mobile, tablet, or desktop device
+ * @param  {String} userAgent User agent string detected from headers
+ * @return {Object} Object of which category the devices falls within


you can drop the JSDoc tags now that you have Flowtypes - don't want to have to keep track of redundant type annotations

mmcgahan · 2017-12-28T20:21:41Z

packages/mwp-core/src/renderers/server-render.jsx

+	}
+	// parses user agent string to determine if user is on mobile / tablet / desktop device
+	const device = new MobileDetect(userAgent);
+	const isMobile = device.mobile() !== null && device.phone() !== null;


is this sufficient?

const isMobile = Boolean(device.phone())

if device.phone() returns a value, then do you also need to know that device.mobile() returns a value?

mmcgahan · 2017-12-28T20:24:50Z

packages/mwp-core/src/renderers/server-render.jsx

+	// parses user agent string to determine if user is on mobile / tablet / desktop device
+	const device = new MobileDetect(userAgent);
+	const isMobile = device.mobile() !== null && device.phone() !== null;
+	const isTablet = device.tablet() !== null;


nitpick, but !== null sort of reads like a double negative to me. IMO Boolean(device.tablet()) is more clear

h-will-h · 2017-12-28T23:12:28Z

Big picture, I think this looks good 👍 .

…W-2907_uas

mmcgahan · 2017-12-29T01:32:35Z

packages/mwp-app-render/src/components/connectWithMatchMedia.test.jsx

+	});
+	const connectWithMatchMedia = shallow(
+		<TestComponentConnectWithMatchMedia />,
+		{ context: { store: mockStore } }


mmcgahan · 2017-12-29T01:33:08Z

packages/mwp-app-route-plugin/src/route.test.js

 		const request = {
-			response: Boom.create(errorCode, errorMessage),
+			response: response,


nitpick, use object shorthand { response, ... }

mmcgahan · 2017-12-29T01:36:23Z

packages/mwp-core/src/renderers/server-render.jsx

+	const userAgent =
+		process.env.NODE_ENV === 'production'
+			? headers['x-ua-device']
+			: headers['user-agent'];


inline comment on these two lines would be very useful

? headers['x-ua-device'] // set by fastly : headers['user-agent']; // fallback to the real UA

…nto MW-2907_uas

jaclynj requested review from h-will-h and mmcgahan December 26, 2017 17:47

jaclynj commented Dec 26, 2017

View reviewed changes

Lori Hutchek and others added 6 commits December 26, 2017 13:16

adding mobile detect plugin + adding data to config object in state

1ec919b

add connectedWithMatchMedia HOC and test

f60a404

update tracking and server render user agent detection

b961fff

update vary header and mwp-consumer readme

b1215d5

update comments

d2a7550

fix yarn lock

12a674d

jaclynj force-pushed the MW-2907_uas branch from 6759aa3 to 12a674d Compare December 26, 2017 18:17

mmcgahan suggested changes Dec 27, 2017

View reviewed changes

mmcgahan reviewed Dec 27, 2017

View reviewed changes

jaclynj added 12 commits December 27, 2017 15:15

remove mobile-device from root dependency and add to mwp-core

0bd2b8d

fix prettier in mwp-core server-render file

0d8cafe

Merge branch 'master' of github.com:meetup/meetup-web-platform into M…

e11ea6a

…W-2907_uas

update to remove false from mobile-detect

4360ecf

small refactor of server-render config for detecting media based on ua

9c78bb1

remove unneeded checks from route plugin

cc0c695

update mwp-consumer readme

db63080

add state.config.media to flow typed

3f7e6de

update with changes to state config, use flow static type checking

f522440

update connectedWithMatchMedia test and snap

a98dd41

update with prettier

7295318

update comment on connectedWithMatchMedia

cb97895

jaclynj commented Dec 28, 2017

View reviewed changes

fix yarn

0e03775

mmcgahan suggested changes Dec 28, 2017

View reviewed changes

jaclynj added 2 commits December 28, 2017 19:37

updates based on fastly changes and pr feedback

f7633eb

make prettier

2df672d

jaclynj added 6 commits December 28, 2017 19:44

Merge branch 'master' of github.com:meetup/meetup-web-platform into M…

da20eca

…W-2907_uas

run yarn after yarn.lock merge conflict

be5edf4

fix detecting prod

6278628

rebuild lockfile

4ae742b

remove x-original-user-agent check since we are no longer doing that

55f502b

reset yarn.lock

8c3f7ab

mmcgahan approved these changes Dec 29, 2017

View reviewed changes

mmcgahan and others added 3 commits December 29, 2017 14:39

build mwp-core lockfile from clean node_modules

b245f04

add comment and es6 shorthand

b6a82c2

Merge branch 'MW-2907_uas' of github.com:meetup/meetup-web-platform i…

27f1dfd

…nto MW-2907_uas

h-will-h approved these changes Dec 29, 2017

View reviewed changes

jaclynj merged commit fa6dd93 into master Dec 29, 2017

chenrui333 deleted the MW-2907_uas branch September 16, 2019 22:36

Mw 2907 uas #423

Mw 2907 uas #423

Conversation

jaclynj commented Dec 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaclynj Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmcgahan Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaclynj Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-will-h commented Dec 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaclynj commented Dec 26, 2017 •

edited

Loading

jaclynj Dec 27, 2017 •

edited

Loading

mmcgahan Dec 27, 2017 •

edited

Loading

jaclynj Dec 28, 2017 •

edited

Loading