Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Make files organisation clearer - Resolves #154 #212

Merged
merged 3 commits into from
May 18, 2017
Merged

Make files organisation clearer - Resolves #154 #212

merged 3 commits into from
May 18, 2017

Conversation

alepop
Copy link
Contributor

@alepop alepop commented May 14, 2017

Resolve #154

  • make file camelCased
  • merge package.json
  • move conf to root
  • add missed eslint package to project

@Isabello
Copy link

Seems ok. It wont pass on Jenkins/Branch. PR-Merge is good enough.

Copy link
Contributor

@slaweet slaweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alepop thank you for the PR, overall it looks very good. Though, there are some issues:

  • It would be great if you could also reflect the changes in README.md, e.g. remove cd src
  • some of the commands defined in package.json result in an error (maybe just for me), namely: npm run dev and npm run start. Can you check, please?
  • Jenkinsfile needs to be updated. Something like: cd $WORKSPACE instead of cd $WORKSPACE/src

@karmacoma karmacoma modified the milestone: Version 1.0.0 May 15, 2017
@karmacoma karmacoma changed the title Make files organisation clearer Make files organisation clearer - Resolves #154 May 15, 2017
@alepop
Copy link
Contributor Author

alepop commented May 15, 2017

@slaweet try to remove node_modules and reinstall npm packages. I do not have a problem with npm scripts on my local machine.

@slaweet
Copy link
Contributor

slaweet commented May 15, 2017

@alihaghighatkhah can you please try to run npm run dev on this PR.

The following snippet should do it:

 git clone [email protected]:alepop/lisk-nano.git alepop-nano
cd alepop-nano/
git checkout 154-make-files-organisation-clearer
npm install
npm run dev

But in my case, it results in the following error:

> [email protected] dev /Users/slaweet/git/alepop-nano
> webpack-dev-server --host 0.0.0.0 --profile --progress

 70% 1/1 build modulesevents.js:161
      throw er; // Unhandled 'error' event
      ^

Error: listen EADDRINUSE 0.0.0.0:8080
    at Object.exports._errnoException (util.js:1028:11)
    at exports._exceptionWithHostPort (util.js:1051:20)
    at Server._listen2 (net.js:1261:14)
    at listen (net.js:1297:10)
    at doListening (net.js:1396:7)
    at _combinedTickCallback (internal/process/next_tick.js:77:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)
    at Module.runMain (module.js:607:11)
    at run (bootstrap_node.js:422:7)
    at startup (bootstrap_node.js:143:9)
    at bootstrap_node.js:537:3

npm ERR! Darwin 16.4.0
npm ERR! argv "/usr/local/Cellar/node/7.6.0/bin/node" "/usr/local/bin/npm" "run" "dev"
npm ERR! node v7.6.0
npm ERR! npm  v4.1.2
npm ERR! code ELIFECYCLE
npm ERR! [email protected] dev: `webpack-dev-server --host 0.0.0.0 --profile --progress`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] dev script 'webpack-dev-server --host 0.0.0.0 --profile --progress'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the lisk-nano package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     webpack-dev-server --host 0.0.0.0 --profile --progress
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs lisk-nano
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls lisk-nano
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/slaweet/git/alepop-nano/npm-debug.log

@alepop
Copy link
Contributor Author

alepop commented May 15, 2017

@slaweet maybe you already run dev server on 8080 port. I got this error only in that case.

@slaweet
Copy link
Contributor

slaweet commented May 15, 2017

Thanks, @alepop for the hint with already running 8080. Indeed, it was the case.

Though, I'm still struggling with npm run start. That gives me a window saying:

Error launching app
Unable to find Electron app at /Users/slaweet/git/alepop-nano/app

Cannot find module '/Users/slaweet/git/alepop-nano/app'

@reyraa
Copy link
Contributor

reyraa commented May 15, 2017

@slaweet npm run dev works fine for me but npm run start opens a blank view in electron app. no errors though. It might be replaced to base tag.

@alepop
Copy link
Contributor Author

alepop commented May 15, 2017

@alihaghighatkhah I have this issue on the previous commit on 1.0.0 branch. webpack clear command remove main.js file (with electron code) from app directory. And they are some issue with path to the app.js file in index.html generated by index.pug.
@slaweet npm start must be run after npm run build

@slaweet
Copy link
Contributor

slaweet commented May 15, 2017

@alepop There's a fix for the index.html blank page issue merged today:
#222

What should we do about the electron main.js file? What's exactly the problem?

@slaweet
Copy link
Contributor

slaweet commented May 15, 2017

@alepop now I see what you mean - about the main.js problem

@slaweet
Copy link
Contributor

slaweet commented May 15, 2017

@alepop but I still don't understand why webpack clear wasn't deleting those files before?

@alepop
Copy link
Contributor Author

alepop commented May 15, 2017

@slaweet maybe clear command was not worked.
CleanWebpackPlugin have optional exclude parameter but they did not work.
I move electron files to desktop folder and and copy-webpack-plugin to copy them to build folder after clear.

@slaweet
Copy link
Contributor

slaweet commented May 15, 2017

@alepop: after discussing it with @alihaghighatkhah we think that a cleaner way to solve the clear issue is to keep git-tracked files in their current location and update webpack to put the generated files (index.html, app.js and possibly other like *.map) into a new folder under /app, e.g. /app/dist.

@alepop
Copy link
Contributor Author

alepop commented May 15, 2017

@slaweet done.
warning, i delete previous commit from my fork.

@slaweet
Copy link
Contributor

slaweet commented May 18, 2017

@alepop after merging your PR I realised that it was opened against 1.0.0 branch, which didn't contain our latest changes, so we reverted it. Can you please open a new PR against development branch?

@alepop
Copy link
Contributor Author

alepop commented May 18, 2017

@slaweet my bad. Ok.

@slaweet
Copy link
Contributor

slaweet commented May 18, 2017

@alepop not your bad at all, the use of branches is currently a bit messy. We need to make it clear, write down some guidelines. Eventually, we probably should be opening PRs against version branches.

@alepop
Copy link
Contributor Author

alepop commented May 18, 2017

@slaweet do I need to make new branch against development and cherry picks my commits from the previous branch, or simply create new PR (my branch made from 1.0.0 branch. Is this may affect your history?)?

@slaweet
Copy link
Contributor

slaweet commented May 18, 2017

@alepop I think that simply create new PR from your current branch should not affect our history, but Github PR UI is always complaining about branches not being up-to-date, so you should cherry-pick or rebase on top of development.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants