Dev environment setup #2

ynigoreyes · 2018-08-06T16:14:14Z

So there were a lot of things to update and change. I was able to get webpack 4 going and it will build correctly for both production and development.

However, I am not able to get Flow or Jest working (and for some reason w0rp/Ale for vim).
Also, node-sass is a giant performance problem: webpack-contrib/sass-loader#531

Proposal

I was hoping we could use Create React App to bootstrap the project. We would most likey run it ejected after configuring jest, Flow, and Hot Reload so that we could alter the webpack configs like we are used to. After ejecting we should be able to add the absolute path configs from our current webpack scripts and everything should at the least compile. The tests will need some work to get working again, but they won't be broken, logic wise. Only configuration wise.

This saves us a lot of time and heartache in trying to figure out how to get jest and Flow running because it comes baked in. Also, there isn't a bunch of weird dependencies so bundle size will stay small if not get smaller since CRA already comes with uglifyication and compression, which solves TTUSDC/cpceed-webapp#34 and comes with polyfills to support the older browsers which solves TTUSDC/cpceed-webapp#39.

Actually, the bundle size will decrease dramatically. on my hackwestx app, i have antd and socket.io as dependencies, and my bundle size for that app is 155kb + 70kb for antd's css. Our bundle size right now is at 1.6 mb.

NilsG-S · 2018-08-06T20:38:19Z

I don't have a problem with you starting over using CRA. The current project will require so many updates/cleanups that it may be cleaner to restart. As it is, we're probably dragging around unnecessary technical debt. How much of the original code will you be able to keep?

Is sass-loader even necessary for our current configuration? Material-UI uses JSS, so it doesn't seem unreasonable to think it might not be.

NilsG-S

Just a few small clarifications/changes

NilsG-S · 2018-08-06T20:43:11Z

package.json

-    "webpack-dev-server": "2.4.1",
-    "webpack-merge": "3.0.0"
+    "webpack": "^4.16.4",
+    "webpack-cli": "^3.1.0",


Typically webpack-cli is installed globally, isn't it? It's a developer tool, not a dev dependency

NilsG-S · 2018-08-06T20:43:48Z

package.json

-    "webpack-merge": "3.0.0"
+    "webpack": "^4.16.4",
+    "webpack-cli": "^3.1.0",
+    "webpack-dev-middleware": "^3.1.3",


webpack-dev-server uses webpack-dev-middleware in an express server to do what it does. This should be unnecessary

Actually the webpack set up will be replaced completely by CRA's so this will change all together

Sounds good. I just wasn't sure whether you wanted to merge these changes in before integrating CRA

Well, i can start the integration now and submit another pull req when EVERYTHING is done. I just like to keep the merges incremental. Granted, going to CRA is definetly incremental... What's would you think is best?

Incremental is definitely better. But if so, I would prefer you fix these nitpicks before I approve

NilsG-S · 2018-08-06T20:46:44Z

package.json

+    "babel-preset-env": "1.7.0",
+    "babel-preset-flow": "6.23.0",
+    "babel-preset-react": "6.24.1",
+    "babel-preset-stage-2": "6.24.1",


What does this get us that env doesn't?

Flow and arrow functions as methods in classes. This also allows us to set initial state outside of the constructor so that we don't have to use the constructor super boilerplate.

NilsG-S · 2018-08-06T20:47:55Z

.babelrc

@@ -1,10 +1,11 @@
 {
  "presets": [
-    "es2015",
-    "react"
+    "env",


Don't you typically have to pass some configuration options to env?

Nope. Works out of the box

NilsG-S · 2018-08-06T20:49:06Z

.eslintrc.js

@@ -3,15 +3,16 @@ module.exports = {
  "env": {
    "mocha": true,
    "node": true,
-    "browser": true,
+    "browser": true


lol, you don't like commas on final lines? My linter typically does

No i do like commas on the final lines, im just not sure as to why some did not have it to begin with. Usually it formats on save and fixes stuff like this but oops

NilsG-S · 2018-08-06T20:51:07Z

src/layout/Auth/AuthContainer.jsx

-  }
-
-  handleRegister(data) {
+  state = {


Is this the preferred way of doing things these days? Moving state and binding out of the constructor, that is

NilsG-S · 2018-08-06T20:56:27Z

webpack/base.js

-      commonPath
-    ]
+      commonPath,
+      testPath,


I assume this is just for Karma. From my recollection Jest has it's own module paths.

https://github.com/CodeSpaceHQ/telescreen/blob/master/sub-projects/hub/jest.server.js

Correct. and again, webpack will change all together

ynigoreyes · 2018-08-06T20:57:21Z

well actually i will be able to keep ALL the code in src, test and common. The webpack config we have right now is going to have to be merged manually if not replaced completely.

NilsG-S · 2018-08-06T20:59:00Z

Sounds good. Is there not some hot-reloading/flow support that has to be changed in the code?

ynigoreyes · 2018-08-06T21:04:25Z

the flow set up is a quick install of flow-bin as a devDep and running Flow init. Hot Reloading set up is the same all around

NilsG-S · 2018-08-06T21:11:59Z

So do you want me to wait until you finish adding CRA stuff to this PR?

ynigoreyes · 2018-08-06T21:13:13Z

Yeah thats fine. I can start the migration now

Significant changes

ynigoreyes · 2018-08-07T23:45:36Z

So i didn't set up flow because it got really mad at me when we tried using the absolute paths that were set up with webpack. And what we had with PropTypes was working completely fine

ynigoreyes · 2018-08-07T23:51:21Z

src/layout/NavBar/components/NavBar.jsx

@@ -111,10 +111,4 @@ NavBar.propTypes = {
  logout: PropTypes.func.isRequired,
 };



So the linter got mad when we had a requiredProp along with a default value. If the user was not passed, then webpack would not compile altogether, so there was no need to add a default prop value

NilsG-S

Looks good for the most part. There were a few things that occurred multiple times which I only commented on once. The exported functional components and accessing state in setState, from what I recall. Would you rather that I comment on each individual occurrence? It might make them easier to find.

NilsG-S · 2018-08-09T11:42:23Z

.eslintignore

@@ -1 +1,4 @@
 **/*{.,-}min.js
+/public
+/config/**
+registerServiceWorker.js


Is there something about service workers that eslint disagrees with?

Yeah, lines 2 and 3 are not needed anymore. The service worker is all over the place. It'm mainly so serve content offline. That is all is seems to do but that boilerplate from CRA. However, it is not styled in any way really, but i don't want to touch it since it is boilerplate from CRA

NilsG-S · 2018-08-09T11:43:56Z

.gitignore

-build/*.json
-build/*.js
+# production
+/build


This should be public now, right?

Well actually you want to version the public folder because that contains you index.html and the service worker. CRA builds from the public folder into the build folder. Yeah it through me off the first time too

NilsG-S · 2018-08-09T11:51:46Z

config-overrides.js

@@ -0,0 +1,7 @@
+const rewireReactHotLoader = require('react-app-rewire-hot-loader')


Do we need react-app-rewired at the moment?

yes, for stateful hot reloading in the development env

NilsG-S · 2018-08-09T11:53:06Z

package.json

+    "babel-preset-env": "1.7.0",
+    "babel-preset-stage-2": "6.24.1",
+    "chai": "4.1.2",
+    "cross-env": "^5.2.0",


Do we still need cross-env at this point?

No we do not, removing now

NilsG-S · 2018-08-09T11:57:34Z

public/index.html

+    <meta charset="utf-8">
+    <!-- Google Styling -->
+    <link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Roboto:300,400,500">
+    <link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons">


Are you doing CDN only, or are you also planning to host the files?

yeah I'm just going to use CDN only, just to keep bundle small, is there any good reason why we should host it?

It's not a big deal. For our purposes this should be fine. If you're concerned that the CDN might go down at some point in the future, it's nice to have self-hosted files as a backup.

NilsG-S · 2018-08-09T12:01:23Z

public/manifest.json

@@ -0,0 +1,15 @@
+{
+  "short_name": "React App",


You should probably change these

NilsG-S · 2018-08-09T12:06:52Z

src/layout/Auth/components/Auth.spec.jsx

+
+describe('Auth.jsx', () => {
+  it('Changes tabs', () => {
+      const authTest = shallow(<Auth authCancelled={() => {}} />);


This spacing looks kinda weird

NilsG-S · 2018-08-09T12:14:02Z

src/layout/NavBar/components/NavBar.jsx

+// Use the navigate and logout props
+
+// TODO: Render the auth view in a modal instead
+export const MenuButton = (props) => {


Do these need to be exported?

NilsG-S · 2018-08-09T12:15:39Z

src/layout/NavBar/components/NavBar.jsx

+  toggleAuth: PropTypes.func.isRequired,
+};
+
+const styles = {


We should consider putting styles in a different file. It's not a big deal here, but I've seen style objects get really big. What do you think?

Well i mean I really don't want to make a whole other file for this one in particular. Most of the styling is done by the component library so to make a new file for maybe 5 lines of uncondensed style might just be clutter. For the Content and Body of the pages, we should make a styles object in a different file.

Sounds good

NilsG-S · 2018-08-09T12:16:17Z

src/layout/NavBar/components/NavBar.jsx

+  toggleAuth = () => {
+    this.setState({
+      ...this.state,
+      auth: !this.state.auth,


Doesn't this violate no-access-state-in-setstate?

it does, but i fixed it

ynigoreyes · 2018-08-10T04:15:01Z

package.json

@@ -59,6 +59,7 @@
  },
  "jest": {
    "setupTestFrameworkScriptFile": "./src/setupTest.js",
+    "testEnvironment": "node",


For some reason, Jest is running really slow. Supposedly this speeds it up?

ynigoreyes · 2018-08-10T04:15:55Z

src/layout/NavBar/components/NavBar.jsx

@@ -12,7 +12,7 @@ import MenuItem from '@material-ui/core/MenuItem';
 import Menu from '@material-ui/core/Menu';

 import { ModalView } from 'components/';
-import AuthContainer from 'layout/Auth';
+import AuthContainer from 'components/Auth';


Moved it to components since we reuse this component

ynigoreyes added 6 commits August 4, 2018 21:47

using debug instead of custom logger

94623fc

switched to yarn and updated babel for react 16

21a5623

updating to webpack 4 and yarn

281ea74

webpack now working for production and developement builds

8c62d6f

changing to more friendly app directory names

69dde0d

fixing project structure

d53d1ca

ynigoreyes requested a review from NilsG-S August 6, 2018 16:14

linting webpack and allowed for eslint to work with babel

7a225f5

NilsG-S previously requested changes Aug 6, 2018

View reviewed changes

adding material ui

f5d9009

ynigoreyes added 4 commits August 6, 2018 16:53

moved all files to a CRA

2c0941c

webpack will now compile with previous src code

64fd37b

switched over to jest and new testing structure

f48d739

fixing production build to exclude source maps

eac0d50

linting whole project

0b9f346

ynigoreyes commented Aug 7, 2018

View reviewed changes

ynigoreyes added 5 commits August 8, 2018 02:30

adding Material UI to Navigation Bar

7f5965a

responsive modal

93f800a

unejecting and setup stateful HMR, rewire-react-app, absolute paths

1eb9562

setup testing and production environment

8a66b7a

adding linting capabilities

2ca52b7

NilsG-S requested changes Aug 9, 2018

View reviewed changes

fixing styles and excess dependencies

91d4af8

ynigoreyes commented Aug 10, 2018

View reviewed changes

NilsG-S approved these changes Aug 10, 2018

View reviewed changes

ynigoreyes merged commit 2a86bc8 into master Aug 10, 2018

ynigoreyes deleted the dev-environment-setup branch August 13, 2018 01:06

		@@ -111,10 +111,4 @@ NavBar.propTypes = {
		logout: PropTypes.func.isRequired,
		};

		@@ -0,0 +1,7 @@
		const rewireReactHotLoader = require('react-app-rewire-hot-loader')

Dev environment setup #2

Dev environment setup #2

Conversation

ynigoreyes commented Aug 6, 2018 • edited Loading

Proposal

NilsG-S commented Aug 6, 2018

NilsG-S left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ynigoreyes Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ynigoreyes Aug 6, 2018 • 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

ynigoreyes commented Aug 6, 2018

NilsG-S commented Aug 6, 2018

ynigoreyes commented Aug 6, 2018

NilsG-S commented Aug 6, 2018

ynigoreyes commented Aug 6, 2018

ynigoreyes commented Aug 7, 2018

Choose a reason for hiding this comment

NilsG-S left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ynigoreyes Aug 10, 2018 • 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

ynigoreyes Aug 10, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ynigoreyes commented Aug 6, 2018 •

edited

Loading

ynigoreyes Aug 6, 2018 •

edited

Loading

ynigoreyes Aug 6, 2018 •

edited

Loading

ynigoreyes Aug 10, 2018 •

edited

Loading

ynigoreyes Aug 10, 2018 •

edited

Loading