Version 2 #53

crisu83 · 2017-04-22T15:43:40Z

Bootstrap project with Create React App instead of using custom configuration.

Todo before this can be merged

Add and configure Stylelint @crisu83
Ensure that HMR is working correctly @AriaMinaei
Ensure that Flow is properly configured @AriaMinaei
Rewrite tests with Jest (try codemods) @crisu83
Upgrade to React Router v4 @crisu83
Add Reselect (and rewrite selectors) @crisu83
Go through dependencies
Update documentation

Things you might be wondering about

Why no es2015 or stage-1 Babel presets? Adding these presets has severe performance implications, so we wanted to leave it up to the project to decide which presets it actually needs. See also Consider leaving out babel-preset-es2015 in development. facebook/create-react-app#435
Why Jest? Facebook is using Jest for testing all of their JavaScript and they have made huge improvements to it during the past year. The DE (Developer Experience) is also superior to any other testing framework).
Why Reselect? We have noticed that we need a unified way to select data from the state to ease the work of the developer. Reselect has detailed documentation and it may also increase the performance of the application with memorized selectors.

hugovk · 2017-04-23T07:10:22Z

CI is failing:

$ flow 
sh: 1: flow: not found
error Command failed with exit code 127.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: "flow" exited with 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

crisu83 · 2017-04-23T08:46:06Z

I fixed the build, but I still need to go over all the changes and verify that everything is OK.

crisu83 · 2017-04-23T08:52:03Z

@AriaMinaei @ericnishio @Kotpes @villeristi could you also please go through the changes and approve them? Thank you in advance.

Jalle19 · 2017-04-23T10:46:03Z

.gitignore

@@ -1,5 +1,26 @@
+<<<<<<< HEAD


Merge error

Will fix, this is basically a completely new code base that was rebased on master.

Jalle19 · 2017-04-23T10:46:31Z

.lintstagedrc

@@ -1,5 +1,6 @@
 {
  "*.js": [
+    "git add .",


Because it's one less manual step that speeds up development and makes people work in smaller chunks. https://github.com/okonet/lint-staged#examples

Think about this scenario:

You work on something

You've staged parts of some files and you're ready to commit

You run the linter, and git add . fucks up your stage. Now you have to unstage everything and carefully stage the lines/chunks/files you had before.

I'm also a bit dubious. Did this cause the .gitignore merge errors to be committed? If anything, perhaps it should only add the modified .js files.

https://github.com/okonet/lint-staged#examples says:

Please note, that it doesn’t work well with committing hunks (git add -p).

Indeed, IMO staging/committing is a job for the developer, not the machine.

Jalle19 · 2017-04-23T10:47:34Z

README.md

@@ -10,7 +10,11 @@

 ## Why do I want this?

+<<<<<<< HEAD


Lots of merge errors here

Jalle19 · 2017-04-23T10:50:00Z

package.json

+    "redux-actions": "^2.0.2",
+    "redux-saga": "^0.14.7"
+  },
+  "devDependencies": {


IMO everything that is not related to webpack-dev-server, linting etc. should be in dependencies.

But why? We should be installing dev dependencies during CI and only leave them out when building production.

We've discussed this once already. The fact is that there are no "production dependencies" since the production build is just a bunch of .js files and some assets.

Currently in order to build the production bundle you have to install every single dependency, including webpack-dev-server.

You have to ask yourself "what can I do if I run yarn install --production"? With the current package.json you will not be able to do anything, not even, i.e. none of the commands here will work (AFAIK):

+ "scripts": { + "ci": "CI=true run-p flow lint-staged test", + "flow": "flow", + "lint": "eslint src --fix", + "lint-staged": "lint-staged", + "precommit": "lint-staged", + "stylelint": "stylelint 'src/**/*.css'", + "start": "node scripts/start.js", + "build": "node scripts/build.js", + "test": "node scripts/test.js --env=jsdom" + },

ericnishio · 2017-04-24T08:36:34Z

README.md


 ```bash
-yarn run compile
+yarn run build


Or simply yarn build.

ericnishio · 2017-04-24T08:38:18Z

config/env.js

+      env[key] = process.env[key];
+      return env;
+    }, {
+      // Useful for determining whether we’re running in production mode.


Overall, I think the inline documentation is too verbose.

It's all from Create React App with as few modifications as possible.

ericnishio · 2017-04-24T08:40:11Z

config/paths.js

@@ -0,0 +1,80 @@
+'use strict';
+
+var path = require('path');


Since we're no longer supporting older versions of Node, we should be able to replace all instances of var with const (and let if needed).

ericnishio · 2017-04-24T08:42:15Z

config/polyfills.js

@@ -0,0 +1,16 @@
+'use strict';


Is this needed?

ericnishio · 2017-04-24T08:42:57Z

config/webpack.config.dev.js

+var paths = require('./paths');
+
+
+


Too many empty lines.

crisu83 · 2017-04-26T07:07:22Z

@Jalle19 let's look at the dependencies today when we meet face to face.

hugovk · 2017-04-26T08:33:16Z

src/index.js

+import React, {createElement} from 'react'
+import {render} from 'react-dom'
+import {AppContainer} from 'react-hot-loader'
+import {browserHistory} from 'react-router'


'browserHistory' is defined but never used.

Jalle19 · 2017-04-27T07:59:36Z

+1 from me as long as rm -rf node_modules && yarn install --production && yarn run compile works

AriaMinaei · 2017-04-27T15:08:13Z

.gitignore

-/node_modules/
-/dist/
+# See https://help.github.com/ignore-files/ for more about ignoring files.
+


Why are we committing the build folder to the repo? @crisu83

We are not?

* Bootstrap with Create React App * Add Stylefmt and Stylelint * Wrote tests with Jest * Fix Flow configuration * Upgrade to React Router v4 * Remove Node versions 4 and 5 from Travis build * Add Reselect and rewrote selectors * Fix Code Climate configuration * Remove unused .csslintrc * Remove unused routes.js * Update README.md * Add .eslintrc * Update Code Climate configuration * Improve types (#54) * Move components into a subdirectory * Simplify bootstrap * Fix import path * Move build dependencies * Fix hyperlink * Update documentation * Add /decls to .eslintignore

crisu83 force-pushed the next branch from f080988 to e1eb3a6 Compare April 23, 2017 08:16

crisu83 assigned AriaMinaei, villeristi, Kotpes and ericnishio Apr 23, 2017

crisu83 added documentation enhancement labels Apr 23, 2017

crisu83 requested review from AriaMinaei, villeristi, Kotpes and ericnishio April 23, 2017 08:50

crisu83 assigned crisu83 and unassigned villeristi, Kotpes and ericnishio Apr 23, 2017

Jalle19 reviewed Apr 23, 2017

View reviewed changes

crisu83 changed the title ~~WIP: Bootstrap with Create React App~~ WIP: Version 2 Apr 23, 2017

Bootstrap with Create React App

12e4e7f

crisu83 force-pushed the next branch from e1eb3a6 to 12e4e7f Compare April 23, 2017 18:41

Add Stylefmt and Stylelint

f1b8d90

crisu83 force-pushed the next branch from 5bcee2f to f1b8d90 Compare April 23, 2017 19:39

crisu83 added 6 commits April 23, 2017 23:36

Wrote tests with Jest

e9bd437

Fix Flow configuration

39fef80

Upgrade to React Router v4

7fbec29

Remove Node versions 4 and 5 from Travis build

bd509bb

Add Reselect and rewrote selectors

307ebff

Fix Code Climate configuration

b251cc7

Update Code Climate configuration

ffdaa62

Kotpes approved these changes Apr 24, 2017

View reviewed changes

ericnishio reviewed Apr 24, 2017

View reviewed changes

AriaMinaei and others added 2 commits April 24, 2017 19:05

Improve types (#54)

ead9e87

Move components into a subdirectory

be5a2f1

crisu83 mentioned this pull request Apr 26, 2017

Upgrade to React Router v4 #45

Closed

hugovk reviewed Apr 26, 2017

View reviewed changes

Simplify bootstrap

17cf998

crisu83 force-pushed the next branch from fb4a623 to 17cf998 Compare April 26, 2017 08:58

crisu83 added 2 commits April 27, 2017 10:29

Fix import path

a321e6a

Move build dependencies

d88fb05

Fix hyperlink

28e7cad

crisu83 removed the request for review from villeristi April 27, 2017 09:27

crisu83 force-pushed the next branch 5 times, most recently from 1828fda to 5d52394 Compare April 27, 2017 10:27

Update documentation

6858155

crisu83 force-pushed the next branch from 5d52394 to 6858155 Compare April 27, 2017 10:29

AriaMinaei reviewed Apr 27, 2017

View reviewed changes

Add /decls to .eslintignore

3a8bd9c

crisu83 merged commit b7eb7cf into master May 4, 2017

crisu83 deleted the next branch May 4, 2017 07:10

crisu83 changed the title ~~WIP: Version 2~~ Version 2 May 4, 2017

hugovk mentioned this pull request Oct 3, 2017

Test coverage is missing #68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version 2 #53

Version 2 #53

crisu83 commented Apr 22, 2017 •

edited

Loading

hugovk commented Apr 23, 2017

crisu83 commented Apr 23, 2017 •

edited

Loading

crisu83 commented Apr 23, 2017

Jalle19 Apr 23, 2017

crisu83 Apr 23, 2017

Jalle19 Apr 23, 2017

crisu83 Apr 23, 2017

Jalle19 Apr 24, 2017

hugovk Apr 24, 2017 •

edited

Loading

Jalle19 Apr 24, 2017

Jalle19 Apr 23, 2017

Jalle19 Apr 23, 2017

crisu83 Apr 23, 2017

Jalle19 Apr 24, 2017 •

edited

Loading

ericnishio Apr 24, 2017

ericnishio Apr 24, 2017

crisu83 Apr 24, 2017

ericnishio Apr 24, 2017

ericnishio Apr 24, 2017

ericnishio Apr 24, 2017

crisu83 commented Apr 26, 2017

hugovk Apr 26, 2017

Jalle19 commented Apr 27, 2017

AriaMinaei Apr 27, 2017

crisu83 May 2, 2017

Version 2 #53

Version 2 #53

Conversation

crisu83 commented Apr 22, 2017 • edited Loading

Todo before this can be merged

Things you might be wondering about

hugovk commented Apr 23, 2017

crisu83 commented Apr 23, 2017 • edited Loading

crisu83 commented Apr 23, 2017

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

hugovk Apr 24, 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

Jalle19 Apr 24, 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

crisu83 commented Apr 26, 2017

Choose a reason for hiding this comment

Jalle19 commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisu83 commented Apr 22, 2017 •

edited

Loading

crisu83 commented Apr 23, 2017 •

edited

Loading

hugovk Apr 24, 2017 •

edited

Loading

Jalle19 Apr 24, 2017 •

edited

Loading