Skip to content
This repository was archived by the owner on May 24, 2022. It is now read-only.

WIP: Relates to #81. Update to Electron v3.1.0 #90

Closed
ltfschoen wants to merge 4 commits into from
Closed

WIP: Relates to #81. Update to Electron v3.1.0 #90

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1bd8d8f
feat: Relates to #81. Update to Electron v3.1.0
ltfschoen Jan 9, 2019
8b347e0
fix: Add version to NPM package.json
ltfschoen Jan 9, 2019
73c2042
fix: Update dependencies. See commit details
ltfschoen Jan 9, 2019
2e2d809
review-fix: Updates based on review comments so far
ltfschoen Jan 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
feat: Relates to #81. Update to Electron v3.1.0 
* Update to Electron v3.1.0 - https://github.com/electron/electron/releases

* Update Readme

* Update dependencies versions

* Add .nvmrc
  • Loading branch information
@ltfschoen
ltfschoen committed Jan 9, 2019
commit 1bd8d8f73ab2650c7fa83dec8e8d5681bfd9b488
1 change: 1 addition & 0 deletions .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
10.11.0
Copy link
Collaborator

@amaury1093 amaury1093 Jan 9, 2019

Choose a reason for hiding this comment

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

Why is this needed actually? Is there a package we use that require at least this version of node? Is it redundant with what you added in package.json?

Copy link
Contributor Author

@ltfschoen ltfschoen Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In the Electron Release Notes for v4.0.0 it says:

Bumped minimum supported macOS version to 10.10.

And on this page they mention no more support for 10.9 in Electron v4 https://electronjs.org/blog/electron-4-0#no-more-macos-109-support

So I've assumed that v3.1.0 would have the same requirement. However the release notes for v4.0.1 or v3.1.0 don't mention a minimum version requirement https://github.com/electron/electron/releases

We could change it to 10.10.0 instead

60 changes: 60 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,66 @@ And below are the lower-level packages, used internally, or by advanced users.
| [`@parity/contracts`](/packages/contracts) | [![npm (scoped)](https://img.shields.io/npm/v/@parity/contracts.svg)](https://www.npmjs.com/package/@parity/contracts) | [![README](https://img.shields.io/badge/docs-README-green.svg)](/packages/contracts#readme) | Parity's [contracts](https://github.com/parity-contracts) as ES6 classes. |
| [`@parity/electron`](/packages/electron) | [![npm (scoped)](https://img.shields.io/npm/v/@parity/electron.svg)](https://www.npmjs.com/package/@parity/electron) | [![README](https://img.shields.io/badge/docs-README-green.svg)](/packages/electron#readme) | Control the Parity Ethereum node from Electron. |

### Contributing

#### Dependencies

Install at least `yarn` version 1.4.2 and [Node.js >=10.11.0](https://nodejs.org/en/)

```
yarn --version // Should be at least 1.4.2
```

#### Tests

```
export NPM_TOKEN=''; yarn test
Copy link
Collaborator

@amaury1093 amaury1093 Jan 9, 2019

Choose a reason for hiding this comment

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

No need for export NPM_TOKEN='';. Can you confirm @ltfschoen?

Copy link
Contributor Author

@ltfschoen ltfschoen Jan 10, 2019

Choose a reason for hiding this comment

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

Yes confirmed. It is no longer required

Copy link
Contributor Author

@ltfschoen ltfschoen Jan 13, 2019

Choose a reason for hiding this comment

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

I just created another issue. It looks like this issue came back again

```

#### Build

```
export NPM_TOKEN=''; yarn build
Copy link
Collaborator

@amaury1093 amaury1093 Jan 9, 2019

Choose a reason for hiding this comment

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

No need for export NPM_TOKEN='';

```

#### Maintenance

1. Fork the repo

2. Clone your fork
```
git clone https://github.com/<INSERT_YOUR_GITHUB_USERNAME>/js-libs
```

3. Check outdated dependencies

```
npm outdated
Copy link
Collaborator

@amaury1093 amaury1093 Jan 9, 2019

Choose a reason for hiding this comment

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

yarn outdated

```

4. Create a branch and update any dependencies that it indicates are out of date.
Copy link
Collaborator

@amaury1093 amaury1093 Jan 9, 2019

Choose a reason for hiding this comment

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

I'd prefer that it'd be us who update dependencies. They sometimes make things break. Or even better: implement #28.


```
git checkout -b <INSERT_YOUR_BRANCH_NAME>
```

5. Run tests, linting, and build

```
export NPM_TOKEN=''; yarn test; yarn lint; yarn build
Copy link
Collaborator

@amaury1093 amaury1093 Jan 9, 2019

Choose a reason for hiding this comment

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

No need for export NPM_TOKEN='';

```

6. Push the branch to your fork of the repo

7. Integrate the updated library as a dependency. Example: If you are using the 'electron' package in another project, then update the package.json file to temporarily use your branch instead of the public NPM registry version
Copy link
Collaborator

@amaury1093 amaury1093 Jan 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@ltfschoen I understand now where your errors come from. I would like to remove this step 7: For @parity/* packages, I would advise against doing yarn add github:something into your own project.

The reason is:

  • right now we develop in src/ in js-libs
  • the CI builds, outputs in lib/, and pushes lib/ to npm
  • when you yarn add @parity/* it downloads lib/ from npm into node_modules, and the main file is lib/index.js

However, when you yarn add github:something, it does:

  • clone the repo into node_modules
  • run yarn build inside the cloned folder
  • hopefully, if the build doesn't error, it generates a lib/ folder

Jaco and I used to try to do this method above, however there's always some troubles here and there in the "hopefully" step. That's why I prefer to remove it. Some alternative solutions:

  • (ideal solution) wait for the npm package to be published. If you need to test a package, it's better to add a test directly in this repo, so that the test is self-contained.
  • use npm link
  • do the copy/paste dance:
# in js-libs/
yarn build
cp -r packages/my-package/lib ../fether/node_modules/@parity/my-package/lib
# in fether
yarn start


```
yarn add https://github.com/<INSERT_YOUR_GITHUB_USERNAME>/js-libs.git#<INSERT_YOUR_BRANCH_NAME>
```

8. Create a pull request from your fork of the repo to the upstream master branch


## License

All Parity's JavaScript libraries are open-source software [licensed as MIT](/LICENSE).
4 changes: 4 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
"packages/*"
]
},
"engines": {
"node": ">=10.11.0",
"yarn": "^1.4.2"
},
"scripts": {
"build": "lerna exec yarn build --stream",
"lint": "tslint 'packages/**/*.ts'",
Expand Down
14 changes: 7 additions & 7 deletions packages/electron/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,21 @@
"test": "jest"
},
"dependencies": {
"async-retry": "^1.2.1",
"async-retry": "^1.2.3",
"axios": "^0.18.0",
"checksum": "^0.1.1",
"command-exists": "^1.2.6",
"debug": "^4.1.0",
"electron-dl": "^1.11.0",
"command-exists": "^1.2.8",
"debug": "^4.1.1",
"electron-dl": "^1.12.0",
"promise-any": "^0.2.0",
"sntp": "^3.0.1"
"sntp": "^3.0.2"
},
"devDependencies": {
"@types/async-retry": "^1.2.1",
"@types/checksum": "^0.1.30",
"electron": "^2.0.2"
"electron": "^3.1.0"
},
"peerDependencies": {
"electron": "^2.0.3"
"electron": "^3.1.0"
Copy link
Collaborator

@amaury1093 amaury1093 Jan 9, 2019

Choose a reason for hiding this comment

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

"^2.0.2 | ^3.1.0 | ^4.0.1"

}
}