[AT] - added Webpack build tools, ESLint, etc

[andrew-c-tran]

The Problem/Issue/Bug:

DDEV-UI currently does not have any build tools or static code analysis.

How this PR Solves The Problem:

Added ESLint with AirBNB rules, added webpack watch and hotreload/realtime linting moved dependencies to fit with webpack's strict modularization.

Introducing webpack, we can now transpile code (ES6, JSX, SCSS, etc) as well as with hotrealoading/file monitoring, we can realtime lint our source code. ESLint was selected as a linter, and the Airbnb ruleset is used as they are both the most widely used and generally accepted as standard (https://hackernoon.com/what-javascript-code-style-is-the-most-popular-5a3f5bec1f6f)

Manual Testing Instructions:

You will need two terminal windows, both navigated to project root:
window 1

• npm run webpack-reload
• it will take a few seconds, but you'll then be assaulted with a barrage of linting warnings/errors (to be expected, the correction of these errors are to be handled in the scss and react refactor issues to immediately follow the acceptance of this PR)
• all JS assets will be transpiled to a single "bundle.js" file that should have been generated at /dist/bundle.js
• the last line of the lint report should be for file ui.js, line 54:8 needing a new line. open ui.js in any editor, add an empty blank line at the end of the file and save.
• the linter should automatically re-run and the previous 55 error count for ui.js should have reduced by one and the entry for newline on 54:8 no longer be there.

window 2

• npm start
• open "./js/site-cards.js" in any editor, and change text "Add/Create Site" (line 11:33-48) to anything you'd like and save.
• look at window 1 and notice the build and linting has automatically rerun.
• in the DDEV-UI application window, refresh it by hitting CMD+R
• note that the add/create site card now has your new text in it, verifying that the changes to the javascript source file was automatically re-transpiled and the running instance of the electron app has updated accordingly

Automated Testing Overview:

This is completely transparent to tests and app functionality, so all tests should currently pass and no tests to directly test build tools are needed.

Related Issue Link(s):

#46

Release/Deployment notes:

No new build steps needed

[andrew-c-tran]

kind of snuck this one in here. noticed another on-click fragment and it was pulled out to be consistent is all.

[andrew-c-tran]

Super simple webpack config. Basically states that we have an electron app, that's main entry point is ui.js, and we want all code to be transpiled into a single file called bundle.js that lives in /js/dist

The loader module will then run eslint on all .js files in the js/src directory.

[rfay]

The code changes look OK, except that:

• bundle.js got committed - it needs to be excluded instead
• make clean needs to delete js/dist right?

As far as functionality, it seems to be broken rather dramatically:

• Add/Create site doesn't work at all any more.
• Stop doesn't work
• Browse local files doesn't work.
  AFAICT the entire UI is nonfunctional.

Let me know if I'm completely crazy. But really, please do take each PR for a casual manual spin yourself before inviting review.

Oh, and I did see

WARNING in ./node_modules/cross-spawn/index.js
Module not found: Error: Can't resolve 'spawn-sync' in '/Users/rfay/go/src/github.com/drud/ddev-ui/node_modules/cross-spawn'

Not sure if that's an error or not.

[rfay]

This doesn't catch js/dist does it? Don't we want that excluded?

[rfay]

In fact, I see that js/dist/bundle.js got committed... It should be removed unless I misunderstand.

[rickmanelius]

I haven't tested myself, but if base functionality is breaking then this further underscores the need to get our integration tests in place as soon as we get the first priorities of the v0.3 phase in place. Until that's the case, we need to do some basic tire kicking to ensure nothing bombs out. @andrew-c-tran Are you able to regenerate the errors that @rfay is noting with respect to the changes brought in by this PR?

[andrew-c-tran]

Alright did some digging and regarding that warning @rfay , a dependency of a node module that is being used in the project (cross-spawn) is attempting to import/require a method (spawn-sync) that is missing from node <= v0.10. It's enclosed in a try/catch block, so the logic is that at runtime it will attempt to require the module, and if it's missing the call will fail and it will warn the end user. This works when being run inside of a javascript engine, but why it's warning is that webpack is just transpiling the files...it's not actually executing the try/catch logic and is warning us that that library is nowhere to be found (as it should not be in a modern node environment.) I then found an open PR for cross-spawn that would fix this warning by completely removing v0.10 support and thus removing the require check, and have asked the developer when they intend to merge (moxystudio/node-cross-spawn#83). Node 0.11 was released in march of 2013, and current LTS is v8.9, with edge being v9.4, so I think it's very safe to assume that all our build boxes won't be impacted by this change when the library is updated.

Testing notes:
Linting was disabled/commented out to be able to check this in an build it. By design, a build will fail if the linting does not pass. In our discussion, we agreed that it would be ineffective to correct the issues that linting failed as we are going to be changing much of this code in the react and scss refactors immediately following this story.

To re-enable realtime linting, open webpack.config.js and remove the comment markers on lines 10 and 21. This will reenable linting as part of the webpack process and will cause make build to fail as there are linting errors, and will autolint as usual when npm run webpack-reload is run.

[rfay]

This is almost ready to go. Let's simplify the Makefile so we're not being repetitive. While there, why don't we add targets for what we're normally doing (the short ddev-ui launch) so that it's done predictably. I'm thinking add an "npmstart" target that does the npm stuff that you need. Then a make clean npmstart would be an easy testing technique. If you want me to make a commit with these changes to the Makefile I will.

[rfay]

This should go in the npminstall target, doesn't make sense to duplicate it here. Let's rename npminstall to npmsetup and put both actions in there.

[andrew-c-tran]

Sure, if you're in there already and can make those make file changes easily then please do. I can get to it tomorrow but am away from a computer right now

…

On Jan 21, 2018 2:23 PM, "Randy Fay" ***@***.***> wrote: ***@***.**** requested changes on this pull request. This is almost ready to go. Let's simplify the Makefile so we're not being repetitive. While there, why don't we add targets for what we're normally doing (the short ddev-ui launch) so that it's done predictably. I'm thinking add an "npmstart" target that does the npm stuff that you need. Then a make clean npmstart would be an easy testing technique. If you want me to make a commit with these changes to the Makefile I will. ------------------------------ In Makefile <#78 (comment)>: > @echo "Building $@" npm run build-linux darwin: npminstall + npm run webpack This should go in the npminstall target, doesn't make sense to duplicate it here. Let's rename npminstall to npmsetup and put both actions in there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#78 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIi5ayJ2sH6RL5MAimvUsC0yjPjNJG4Uks5tM6rmgaJpZM4RgL3l> .

[rfay]

I added the suggested update to the Makefile in ca43b81 - This simplifies it slightly. The clean target had to be fixed because it wasn't removing the right "dist", and there's a new npmstart target that can be used for local testing. make npmstart does what needs to be done. make clean npmstart will hit the clean target first and then do the npmstart-related activities.