Fix Electron linting errors #929

2018-01-08T06:24:12+01:00

IGassmann commented

2018-01-08 06:24:12 +01:00

(Migrated from github.com)

This pull request fixes #397 and:

Adds brew update to build script for getting latest dependencies versions
Cleans .gitignore to remove IDE specific ignores and old rules.
Renames package-json name field to be compliant with npm and yarn rules, and adds productName to build
Replaces dependency keytar with keytar-prebuild until https://github.com/atom/node-keytar/pull/67 is not merged. This simplifies the build process on Windows. There's no need anymore for Python and Windows building tools.

This pull request fixes #397 and: - Adds `brew update` to build script for getting latest dependencies versions - Cleans [`.gitignore`](https://github.com/lbryio/lbry-app/commit/7068ebaf57ed00f316b313b26574f788f513d280#diff-a084b794bc0759e7a6b77810e01874f2) to remove IDE specific ignores and old rules. - Renames `package-json` name field to be compliant with npm and yarn rules, and adds productName to build - Replaces dependency `keytar` with `keytar-prebuild` until https://github.com/atom/node-keytar/pull/67 is not merged. This simplifies the build process on Windows. There's no need anymore for Python and Windows building tools.

lyoshenka (Migrated from github.com) reviewed 2018-01-08 19:40:56 +01:00

lyoshenka (Migrated from github.com) left a comment

How much of build.sh can we put into yarn build. I'd love to drop that script, or at least work towards dropping it.

How much of `build.sh` can we put into `yarn build`. I'd love to drop that script, or at least work towards dropping it.

.gitignore

lyoshenka (Migrated from github.com) commented

2018-01-08 19:38:39 +01:00

this is here because many of us use JetBrains, which creates that dir

.gitignore

					
				@ -21,4 +9,1 @@

				*.pyc

				*.iml

				.#*

lyoshenka (Migrated from github.com) commented

2018-01-08 19:38:15 +01:00

i believe build/venv is still used (by build.sh)

.gitignore

lyoshenka (Migrated from github.com) commented

2018-01-08 19:39:33 +01:00

this has to be here while upload_assets.py is still in use. if you want to rewrite that in JS instead, then we can drop python dependency completely, which would be sweet!

README.md

lyoshenka (Migrated from github.com) commented

2018-01-08 19:37:20 +01:00

why did you get rid of DEPS=true?

build/install_deps.sh

					
				@ -40,8 +40,12 @@ set -eu

				if $LINUX; then

				  INSTALL="$SUDO apt-get install --no-install-recommends -y"

lyoshenka (Migrated from github.com) commented

2018-01-08 19:35:48 +01:00

this only gets run if brew command does not exist. so it will only update when it is installed. is that what you intended?

this only gets run if `brew` command does not exist. so it will only update when it is installed. is that what you intended?

IGassmann (Migrated from github.com) reviewed 2018-01-08 21:50:36 +01:00

.gitignore

IGassmann (Migrated from github.com) commented

2018-01-08 21:50:36 +01:00

I think it's better to keep project related ignore rules into the project .gitignoreand user-specific rules into the user's global .gitignore.

I think it's better to keep project related ignore rules into the project `.gitignore`and user-specific rules into the user's global `.gitignore`.

IGassmann (Migrated from github.com) reviewed 2018-01-08 22:30:04 +01:00

README.md

IGassmann (Migrated from github.com) commented

2018-01-08 22:30:04 +01:00

There's no need anymore for installing dependencies from the build script. The user should install the prerequisites listed in README.md. That is Git, Node, and Yarn. No need anymore for Python for developing.

There's no need anymore for installing dependencies from the build script. The user should install the prerequisites listed in `README.md`. That is Git, Node, and Yarn. No need anymore for Python for developing.

IGassmann commented

2018-01-08 22:33:42 +01:00

(Migrated from github.com)

build.sh is currently only necessary for downloading and extracting the daemon. I'm planning to try to get the daemon as a package.json dependency. It should be done with node-pre-gyp as far as I know.

`build.sh` is currently only necessary for downloading and extracting the daemon. I'm planning to try to get the daemon as a `package.json` dependency. It should be done with `node-pre-gyp` as far as I know.

lyoshenka commented

2018-01-10 23:57:37 +01:00

(Migrated from github.com)

is the app no longer built/released by teamcity? teamcity runs build.sh

lyoshenka (Migrated from github.com) reviewed 2018-01-10 23:58:10 +01:00

README.md

lyoshenka (Migrated from github.com) commented

2018-01-10 23:58:10 +01:00

it would be much better if users ran a script to install dependencies instead of following a README. that way if the dependencies change, no one has to remember to update the README

lyoshenka (Migrated from github.com) reviewed 2018-01-11 00:00:10 +01:00

lyoshenka (Migrated from github.com) left a comment

i hope you tested this on all 3 oses, and made sure it still works with teamcity. if so, lgtm

package.json

lyoshenka (Migrated from github.com) commented

2018-01-10 23:59:19 +01:00

why did this change? will this affect the .deb package that's built? if the name changes, i think ubuntu will think its a different package...

IGassmann (Migrated from github.com) reviewed 2018-01-11 00:42:03 +01:00

package.json

IGassmann (Migrated from github.com) commented

2018-01-11 00:42:03 +01:00

I changed the name to follow the norms. I'm not sure how exactly it'll behave in Ubuntu since I didn't test on it, but it should normally use the productName field declared in the electron-builder.json file and not the package.json's name.

I'll test it to be sure.

I changed the name to follow the [norms](https://yarnpkg.com/lang/en/docs/package-json/#toc-name). I'm not sure how exactly it'll behave in Ubuntu since I didn't test on it, but it should normally use the [`productName` field](https://yarnpkg.com/lang/en/docs/package-json/#toc-name) declared in the `electron-builder.json` file and not the `package.json`'s `name`. I'll test it to be sure.

IGassmann (Migrated from github.com) reviewed 2018-01-15 16:42:13 +01:00

package.json

IGassmann (Migrated from github.com) commented

2018-01-15 16:42:13 +01:00

Debian does identify the app as a new package and doesn't replace the one with the previous name.

I think we should wait for the AppImage switch, which should come with the auto-update PR, before applying this change and then request Linux users that they uninstall their previous install.

Debian does identify the app as a new package and doesn't replace the one with the previous name. I think we should wait for the AppImage switch, which should come with the auto-update PR, before applying this change and then request Linux users that they uninstall their previous install.