Improvements to src/main.js

brodycj commented 5 years ago

Check that npm dependencies are present before other require declarations in src/main.js
Move code to check npm dependencies to src/check-npm-install-util.js utility module
define lazyRequire in src/lazy-require-util.js
var -> const / let in main.js, with vertical space added before some of the const & let declarations
fix to show correct path to run "npm install", if npm dependencies are not present

Before these changes, running cordova-coho/coho without running npm install would result in the following error output:

internal/modules/cjs/loader.js:582
    throw err;
    ^

Error: Cannot find module 'chalk'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:580:15)
    at Function.Module._load (internal/modules/cjs/loader.js:506:25)
    at Module.require (internal/modules/cjs/loader.js:636:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/Users/brodybits/Documents/cordova/cordova-coho/src/apputil.js:21:13)
    at Module._compile (internal/modules/cjs/loader.js:688:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:699:10)
    at Module.load (internal/modules/cjs/loader.js:598:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:537:12)
    at Function.Module._load (internal/modules/cjs/loader.js:529:3)

janpio commented 5 years ago

What are the actual changes here vs. "just" refactoring/extraction?

brodycj commented 5 years ago

I broke the changes down to 4 commits to show actual changes vs refactoring & extraction and listed the changes more clearly in the description. I also changed var -> const throughout main.js, using let a couple times when const did not work.

I hope the updates are clear now. Please don't hesitate to ask if you have any further questions.

janpio commented 5 years ago

Nope, that is pretty much perfect to follow now. Left 2 comments.

brodycj commented 5 years ago

Left 2 comments.

I hope they are resolved ok. I also pushed a main-js-improvements-rebased branch that puts the changes in a nicer order. (git diff main-js-improvements..main-js-improvements-rebased shows no differences)

I would personally favor keeping the code extraction in its own commit to make it easier for others to understand exactly what changed in the future. I would also prefer to keep the non-extraction code changes in 2 commits like I proposed in main-js-improvements-rebased.

brodycj commented 5 years ago

@janpio do you have any more comments? I would like to merge this one soon.

janpio commented 5 years ago

Don't have the time to try that out right now, someone else will have to review and approve. Sorry.

apache / cordova-coho

Improvements to src/main.js #221