Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: enforce style rules #99

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix: enforce style rules #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9a8358e
fix: don't silence fatal or unknown (non-warning) errors
coolaj86 Jun 16, 2023
8e4da24
Chore: set .jshintrc.latedef = "nofunc" (for browsers and node)
coolaj86 Jun 20, 2023
edcc4df
Chore: bump .jshintrc.esversion to 11 (the dashd suite relies on node…
coolaj86 Jun 20, 2023
3374cde
Chore: bump tests to node v18 (to allow use of WebCrypto)
coolaj86 Jun 20, 2023
8b3d4e5
Chore: enforce style rules with 'npm run fmt'
coolaj86 Jun 20, 2023
62c994d
chore: 'npm run fmt' to apply style rules
coolaj86 Jun 20, 2023
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
9 changes: 9 additions & 0 deletions .editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
root = true

[*]
charset = utf-8
indent_style = space
indent_size = 2
trim_trailing_whitespace = true
insert_final_newline = true
end_of_line = lf
5 changes: 4 additions & 1 deletion .github/workflows/test_and_release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jobs:

- uses: actions/setup-node@v2
with:
node-version: '12'
node-version: '18'

- name: Enable NPM cache
uses: actions/cache@v2
Expand All @@ -39,6 +39,9 @@ jobs:
- name: Run jshint
run: npm run jshint

- name: Run fmt
Copy link
Member

Choose a reason for hiding this comment

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

We probably shouldn't format here but make sure that formatting already is correct.

Copy link
Author

Choose a reason for hiding this comment

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

This will cause it to fail if it's not correct. This enforces that the style is followed.

If you don't run this, you can't enforce that it was done before (unless you do something else that's effectively the same).

Copy link
Member

Choose a reason for hiding this comment

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

So it doesn't format itself but only checking that the format is correct?

Copy link
Author

Choose a reason for hiding this comment

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

That's correct. It will exit 1 if there are any changes and exit 0 if there are no changes.

run: npm run fmt

- name: Run tests
run: npm run test

Expand Down
29 changes: 12 additions & 17 deletions .jshintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,42 +1,37 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

We use eslint + airbnb style for all JS packages. Shall we just move to eslint + prettier?

Copy link
Author

@coolaj86 coolaj86 Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Prettier is definitely the standard for JS.

ESLint is more for TypeScript and ECMAScript and other JS derivatives. JSHint is for JS.

ESLint can operate on JS, but it has a boatload of dependencies, very complex configuration, etc.

I personally won't run eslint because it's too risky and insecure due to the supply chain vulns', but if CI/CD can run and report it... meh. Still, there should be a way to apply ESLint that is not incompatible with just JavaScript, and therefore not incompatible with JSHint.

That said, ESLint is better than nothing, but it's nowhere near as good as JSHint for JavaScript. JSHint is a single dependency, no plugins, no formatting, no rewrites, no style, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I think the most important here is a consistency. All JS repos in this org should follow the same code style. Then we should go with the same configuration for eslint what we in https://github.com/dashpay/platform/ and prettier should follow eslint config https://github.com/prettier/eslint-config-prettier

Copy link
Author

@coolaj86 coolaj86 Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

All JS repos in this org should follow the same code style

  1. they don't presently
  2. this fixes this repo as this repo is presently set up
  3. it's a much bigger lift to do the work to transition it to eslint
  4. Opening a can of worms...

Why move from something that has just 2 dependencies to something that has hundreds of dependencies? There are just too many things that go wrong. Too many versions to track. Slower CI/CD times.

If someone else wants to bring in the complexity and then make all the corresponding updates, then let them do that in another PR to transition the repository. But for now, let's just fix what's here.

I don't use eslint because I value both security and ease of use. eslint is an absolute monster. I won't install it on my computer because I'm morally opposed to it - it encourages non-standard features, supply chain security attacks (which have been exploited), and overall bloat.

"esversion": 11,
"bitwise": false,
"browser": true,
"camelcase": false,
"curly": true,
"devel": false,
"eqeqeq": true,
"esnext": true,
"freeze": true,
"immed": true,
"indent": 2,
"latedef": true,
"latedef": "nofunc",
"laxbreak": true,
"newcap": false,
"noarg": true,
"node": true,
"noempty": true,
"nonew": true,
"quotmark": "single",
"regexp": true,
"smarttabs": false,
"strict": true,
"trailing": true,
"undef": true,
"unused": true,
"maxparams": 5,
"maxstatements": 17,
"maxcomplexity": 10,
"maxdepth": 3,
"maxlen": 120,
"multistr": true,
"predef": [
"after",
"afterEach",
"before",
"beforeEach",
"describe",
"exports",
"it",
"module",
"require"
"after",
"afterEach",
"before",
"beforeEach",
"describe",
"exports",
"it",
"module",
"require"
]
}
3 changes: 3 additions & 0 deletions .prettierrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"printWidth": 120
}
29 changes: 15 additions & 14 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
Dashcore Node
============
# Dashcore Node

A Dash full node for building applications and services with Node.js. A node is extensible and can be configured to run additional services. At the minimum a node has an interface to [Dash Core (dashd) v0.13.0](https://github.com/dashpay/dash/tree/v0.13.0.x) for more advanced address queries. Additional services can be enabled to make a node more useful such as exposing new APIs, running a block explorer and wallet service.

Expand All @@ -23,6 +22,7 @@ Some plugins are available :
- Insight-UI : `./bin/dashcore-node addservice @dashevo/insight-ui`

You also might want to add these index to your dash.conf file :

```
-addressindex
-timestampindex
Expand All @@ -36,25 +36,25 @@ npm install @dashevo/dashcore-node
```

```javascript
const dashcore = require('@dashevo/dashcore-node');
const config = require('./dashcore-node.json');
const dashcore = require("@dashevo/dashcore-node");
const config = require("./dashcore-node.json");

let node = dashcore.scaffold.start({ path: "", config: config });
node.on('ready', function () {
console.log("Dash core started");
node.services.dashd.on('tx', function(txData) {
let tx = new dashcore.lib.Transaction(txData);
console.log(tx);
});
node.on("ready", function () {
console.log("Dash core started");

node.services.dashd.on("tx", function (txData) {
let tx = new dashcore.lib.Transaction(txData);
console.log(tx);
});
});
```

## Prerequisites

- Dash Core (dashd) (v0.13.0) with support for additional indexing *(see above)*
- Dash Core (dashd) (v0.13.0) with support for additional indexing _(see above)_
- Node.js v8+
- ZeroMQ *(libzmq3-dev for Ubuntu/Debian or zeromq on OSX)*
- ZeroMQ _(libzmq3-dev for Ubuntu/Debian or zeromq on OSX)_
- ~50GB of disk storage
- ~1GB of RAM

Expand Down Expand Up @@ -95,7 +95,6 @@ There are several add-on services available to extend the functionality of Bitco
- [Bus](docs/bus.md) - Overview of the event bus constructor
- [Release Process](docs/release.md) - Information about verifying a release and the release process.


## Setting up dev environment (with Insight)

Prerequisite : Having a dashd node already runing `dashd --daemon`.
Expand All @@ -105,13 +104,15 @@ Insight-api (optional) : `git clone https://github.com/dashevo/insight-api -b de
Insight-UI (optional) : `git clone https://github.com/dashevo/insight-ui -b develop`

Install them :

```
cd dashcore-node && npm install \
&& cd ../insight-ui && npm install \
&& cd ../insight-api && npm install && cd ..
```

Symbolic linking in parent folder :

```
npm link ../insight-api
npm link ../insight-ui
Expand Down
Loading