Initial implementation of Login and Registration components #1

zacklitzsinger · 2016-12-02T02:02:13Z

This is a large batch of changes that provides a jumping off point for future development. Moving forward, features will be pulled in individually - however, early on there are so many things that are more experimental rather than specific features.

Changelog:

Linting rules were altered. I went ahead and set up a precommit hook that runs linting so all commits will have to pass linting.
Basic server to use when developing with hot module reloading. The server is run through babel-node, so it follows ES6 conventions.
GraphQL backend (currently to manage lists of users) along with a simple flat file database. This is not secure, so be careful about what information you put into it.
Simple Redux implementation with actions that call the API to login and create users.
Modified the file structure so that actions, reducers, etc. are all alongside the component they relate to.
We now support Stage 2 Babel features.

fix npm version typo in DEVELOPMENT.md file use babel-lint instead of eslint for stage2 features add scaffolding for redux store remove lib/index.js build file from repo basic styling for login widget

actions can now talk to graphql and get responses back serve webpack using middleware instead of dev server

registration action that talks to the server using graphql webpack dev hot reloading

… mongodb

break schema up into smaller files add password to register

fetch now passes the key along correctly

jessemao · 2016-12-05T19:01:18Z

server/index.js

+
+const app = express();
+
+const PORT = 3000;


should we add a node_env for port so that other people can change it easily.

If we want to do this, I would prefer to add it as an optional argument. --port=3000 for example.

Now that I thought about this a little more, I find it unlikely that developers will want to change this port, at least when developing this project. On their own server, they definitely would want that capability.

jessemao · 2016-12-05T19:05:40Z

server/index.js

+app.use('/graphql', graphqlHTTP((req) => {
+    return {
+        schema: schema,
+        rootValue: req,


we can do
app.use('/graphql', graphqlHTTP({
schema: MyGraphQLSchema,
graphiql: true
}));
The req will be passed in as context. I don't think we want to pass req as rootValue here. rootValue should be something totally customized, like root resolve functions

Ahh, I didn't realize the request was provided automatically. Thanks!

jessemao · 2016-12-05T19:07:12Z

server/index.js

+
+// Login endpoint. This authenticates the session. We could instead do
+// this using GraphQL.
+app.post('/login', function (req, res) {


I prefer using GraphQL for auth part as well, since we are going to use GraphQL. Not a fan of mixing both.

I considered doing this. However, authenticating the token in graphQL might get confusing. Personally, I'm okay with doing login using a normal endpoint and doing everything else in graphQL. I would also be okay with doing authentication in graphQL if it is relatively straightforward.

jessemao · 2016-12-05T19:12:29Z

server/schema/queries/auth.js

+    auth: {
+        type: GraphQLBoolean,
+        resolve (_, params, req) {
+            console.log('session: ', req.session);


jessemao · 2016-12-05T19:13:15Z

server/schema/queries/user/user.js

+    type: UserType,
+    args: {
+        uid: {
+            name: 'uid',


rename the name to uid?

jessemao · 2016-12-05T19:16:14Z

server/schema/types/create_user.js

@@ -0,0 +1,10 @@
+import {GraphQLInputObjectType, GraphQLNonNull, GraphQLString} from 'graphql';


suggest we change the file name to user_input.js, because this is the type for generic user, we can also use it for update_user

jessemao · 2016-12-05T19:17:57Z

src/App/index.js


 export default class App extends Component {
+    static propTypes = {


I don't think the props children needed to be declared here. prefer to remove it.

You do actually need to declare children propTypes: jsx-eslint/eslint-plugin-react#7. If you meant that App doesn't need children, it does because the whole point is to wrap other components with the provider and base styles.

yea, I didn't know the children is required for eslint. it's okay then.

jessemao · 2016-12-05T19:19:41Z

src/App/root_reducer.js

@@ -0,0 +1,8 @@
+import {combineReducers} from 'redux';
+import login from '../Login/reducer';


we can probably alias a root folder to src, so instead, we write code like import login from '../Login/reducer', we write import login from 'Login/reducer'

We can do this, but it has downsides. Notably, it becomes more confusing which modules are internal vs external dependencies. Let's get other opinions on this. @dennismphil?

you don't really need that I guess, easy way to do is to set a alias in webpack file. like

{ resolve: { root: ... } }

and we can separate the external/internal packages when we import than by put the external package in front.

Agree with @jessemao it's not pretty to look. But good point about the downside of being confused. I prefer to keep it as is.

jessemao · 2016-12-05T19:21:24Z

src/Login/action_types.js

@@ -0,0 +1,6 @@
+export default {
+    USERNAME: 'LOGIN_USERNAME',


I personally prefer to have action_types fields match up with its string value. Like LOGIN_USERNAME: LOGIN_USERNAME

The reason to do it this way is so that you can import all login actions together. I find importing individual action types to be a little cumbersome. It requires the imports to be changed every time you add a new action.

There's another method may be better.

export const USERNAME = 'LOGIN_USERNAME'; --- import * as LOGIN from './action_types';

I think this will still allow you to reference the action as LOGIN.USERNAME.

jessemao · 2016-12-05T19:26:50Z

src/Login/index.js

+                <div className={theme.property}>
+                    <label className={theme.propertyLabel} htmlFor='uname'>{constants.USERNAME}</label>
+                    <input type='text' id='uname' placeholder={constants.USERNAME} required={true} value={username} onChange={(e) => { onUsernameChange(e.target.value); }}/>
+                    <input type='checkbox' id='remember' required={true} value={remember} onChange={(e) => { onRememberToggle(e.target.checked); }}/>


we should be able to give our user the ability to change the id.

You mean developers right? I agree, but I think we should have sensible defaults and base the ids of subcomponents in a component off a base id. Like so:

const {id} = this.props; <input type='text' id=`${id}-uname` />

yeah, we should totally have a default id for it with the ability to change them as we want.

jessemao · 2016-12-05T19:27:05Z

src/Login/index.js

+                </div>
+                <div className={theme.property}>
+                    <label className={theme.propertyLabel} htmlFor='pword'>{constants.PASSWORD}</label>
+                    <input type='password' id='pword' placeholder={constants.PASSWORD} required={true} value={password} onChange={(e) => { onPasswordChange(e.target.value); }} />


jessemao · 2016-12-05T19:28:16Z

src/README.md

@@ -0,0 +1,7 @@
+# Component library
+
+Source files are bundled using Webpack and exported in `../lib`. Included components are defined here.


better to use abs path here.

jessemao · 2016-12-05T19:29:07Z

src/Register/action_types.js

@@ -0,0 +1,6 @@
+export default {
+    USERNAME: 'REGISTER_USERNAME',


same here, match up field name and value.

jessemao · 2016-12-05T19:29:15Z

src/Register/actions.js

+        const res = await query({query: `mutation {
+            createUser(data:{uid:'${name}', password:'${password}', fullName:'${fullName}'})
+        }`});
+        console.log('res: ', await res.json());


jessemao · 2016-12-05T19:30:05Z

src/Register/actions.js

+export function attemptRegister(name, password, fullName) {
+    return async function (dispatch) {
+        const res = await query({query: `mutation {
+            createUser(data:{uid:'${name}', password:'${password}', fullName:'${fullName}'})


we should use format like this if we need to pass in variables.

{ query: `mutation ($userId: ID, $tag: [String]) { updateUser(data:{_id: $userId, tag: $tag}) { _id, tag } }`, variables: JSON.stringify({ userId, tag }) }

jessemao · 2016-12-05T19:40:46Z

src/Register/index.js

+            <div className={theme.container}>
+                <div className={theme.property}>
+                    <label className={theme.propertyLabel} htmlFor='uname'>{constants.USERNAME}</label>
+                    <input type='text' id='uname' placeholder={constants.USERNAME} required={true} value={username} onChange={(e) => { onUsernameChange(e.target.value); }}/>


same as input section

jessemao · 2016-12-05T19:41:23Z

src/constants.js

+
+export default {
+    // Localization
+    USERNAME: 'User name',


I think we should use Username or just User

jessemao · 2016-12-05T19:43:39Z

webpack.config.js

@@ -1,17 +1,11 @@

 const NODE_ENV = process.env.NODE_ENV;


we can probably have a webpack.config.base to include the duplicate codes in both dev and production config files.

We could, but at least for now, the development and production webpack files are different in a lot of subtle but significant ways. For example, development uses hot middleware + react-hot. At some point, production will probably use just a hash for CSS names rather than the BEM class name style. We could consider using webpack-merge.smart, but that is complicated enough that I would rather break it out.

I don't know if you have read this yet, it's really good.
survival webpack

jessemao

overall looks good!

dennismphil · 2016-12-06T18:46:08Z

server/index.js

+    console.log('initial session: ', req.session);
+    const {uid, password} = req.body;
+    const user = db.get('users').get(uid).value();
+    if (!user || user.password != password) {


Use !== instead of !=

we should include this as an error in our eslint json.

dennismphil · 2016-12-06T20:30:01Z

src/dev.js

+import Login from './Login';
+import Register from './Register';
+
+if (document.getElementById('root') == null){


Use strict equals

dennismphil · 2016-12-06T20:34:52Z

webpack.config.js

-    CONFIG = require("./webpack.config.development")
-} else if (NODE_ENV == "production") {
-    CONFIG = require("./webpack.config.production")
+if (NODE_ENV == 'development'){


Strict equals

dennismphil · 2016-12-06T20:35:04Z

webpack.config.js

-    CONFIG = require("./webpack.config.production")
+if (NODE_ENV == 'development'){
+    CONFIG = require('./webpack.config.development');
+} else if (NODE_ENV == 'production') {


Strict equals

dennismphil

Great start!

jessemao · 2016-12-06T21:29:19Z

server/index.js

@@ -15,6 +15,7 @@ import webpackHotMiddleware from 'webpack-hot-middleware';

 const app = express();

+// TODO: Provide as optional argument?
 const PORT = 3000;


what about let's just do const PORT = NODE_ENV.port || 3000;

I'm adding this, but I don't think anyone will use it.

Zack Litzsinger added 15 commits November 22, 2016 19:43

use babel stage2 features

706a321

fix npm version typo in DEVELOPMENT.md file use babel-lint instead of eslint for stage2 features add scaffolding for redux store remove lib/index.js build file from repo basic styling for login widget

hooked login component up to redux

51a3dac

small tweaks

60fa047

basic express server with graphql working

1b678f9

added server logging

ac27492

actions can now talk to graphql and get responses back serve webpack using middleware instead of dev server

basic graphql mutation to create a user

aff9dd6

registration component

510ce8c

registration action that talks to the server using graphql webpack dev hot reloading

write server in es6 with imports

29bd11c

writing server authentication + session storage + user creation using…

73b7075

… mongodb

use lowdb as a simple database

7fd3d08

break schema up into smaller files add password to register

fixed some linting issues

b03eac6

added readmes

467c20f

added authentication demo

0819835

arranged file structure by feature not by type

950bafb

fetch now passes the key along correctly

use single quotes

79cd677

zacklitzsinger added the enhancement label Dec 2, 2016

zacklitzsinger assigned jessemao, tightrough, zacklitzsinger and dennismphil Dec 2, 2016

Merge branch 'master' into graphql-test

c8f8417