Apply template - Closes #568,#569,#570,#571,#572,#573,#575 #558

shuse2 · 2018-02-09T18:22:48Z

Closes #568
Closes #569
Closes #570
Closes #571
Closes #572
Closes #573
Closes #575

AndrewBarba · 2018-02-10T23:05:03Z

package.json

+		"url": "https://github.com/LiskHQ/lisk-js/issues"
+	},
+	"engines": {
+		"node": "=6.12.3",


I have a feeling this would break yarn and maybe npm for any user trying to install this on a newer version of node or npm. Lisk-js should not be required to run on this exact version of node. >=6 I think is more appropriate

Currently lisk core only supports particular version of node.
That is why it is freeze at 6.12.3

But this is not lisk core... the js sdk should be able to run on any version of node or the browser that supports the necessary syntax

Breaks on yarn v1.3.2 but works fine on npm v3.10.10 (which ships with Node.js v6.12.3). @Tosch110 thoughts?

Yes I understand the point. However, due to the fact that, there was change in Buffer behaviour,
lisk-js also supports only v6 now.

I created the follow up issue #561 for this.

@AndrewBarba I'm afraid I don't really understand your points about a swift SDK, maybe there's some context I'm missing? Regarding the current proposal for these fields...

From the npm docs:

You can specify the version of node that your stuff works on:
{ "engines" : { "node" : ">=0.10.3 <0.12" } }

We aren't currently testing multiple versions of node with LiskJS, which is why we're planning to put only one version of node in there. I'm definitely open to the idea that we should allow all node 6 versions. However, there's at least one breaking change in node 8 which breaks LiskJS functionality. So according to the npm documentation—as far as I can see—we definitely shouldn't put a range in engines which allowed the use of node 8.

Do you have any resources which advocate a different way of using the engines field?

yarn is not our recommended package manager, but it does offer the --ignore-engines option if you want to ignore our recommendation as well as the fact that we only test LiskJS with one version of node and that's the only version we currently support.

@willclarktech @shuse2 The original answer to my concern was that lisk core uses Node v6. My point with the swift sdk was that an SDK using the core API to spec has nothing to do with the software versions running on core. Thats the whole point of an api, i call according to spec and I dont need to know what the core machine is using. But now the answer has changed, that apparently lisk js breaks on Node v8 because of a buffer issue. That is completely different (and valid) reason to use the engines options, but the original reason was incorrect usage of the engines options. I currently use lisk-js 0.5.1 on Node v8 extensively, and also on AWS lambda which uses Node 6.10. I bet a lot of others do this as well. So if tests legitimately do not pass on Node v8, then fine, limit to a range of Node 6. But if you leave it as is, very few people are going to be able to install and use this as they currently are

Additionally, this is the main dependency in Lisk Nano which is built on Electron, which uses Node v8. So how could you not support Node v8?

We have to fix the Buffer bug from node (or breaking changes in the returned error) on our side and support a great range of node versions (6.3 - 8 at least) asap. This will be done before the release.

AndrewBarba · 2018-02-10T23:05:15Z

package.json

+	},
+	"engines": {
+		"node": "=6.12.3",
+		"npm": "=3.10.10"


Same comment as above

willclarktech · 2018-02-12T15:01:15Z

.eslintrc.json

@@ -1,5 +1,6 @@
 {
 	"extends": ["lisk-base"],
+	"plugins": ["mocha"],


This should be in test/.eslintrc.json.

willclarktech · 2018-02-12T15:03:19Z

package.json

+		"url": "https://github.com/LiskHQ/lisk-js/issues"
+	},
+	"engines": {
+		"node": "=6.12.3",


Breaks on yarn v1.3.2 but works fine on npm v3.10.10 (which ships with Node.js v6.12.3). @Tosch110 thoughts?

willclarktech · 2018-02-12T15:04:58Z

.gitignore

@@ -58,3 +59,7 @@ dist-node
 browsertest/package.json
 browsertest/browsertest.js
 browsertest/browsertest.min.js
+
+# lock files (required version is npm below 3)


Do we really want to add these? We want to be discouraging people from creating them in the first place.

if user accidentally use npm >= 5, it will automatically adds lock file.
Therefore, I thought it will be good to added to ignorefile until we can support higher version

One thought is whether having to fight with git to keep lockfiles around might encourage people to use the package manager we support to avoid that struggle?

wouldn't it increase the PR discussion every time someone commits the lock file?

Probably. Let's add them here then, but text should be more like Supported package manager is npm 3.

willclarktech · 2018-02-12T15:08:28Z

package.json

+	"license": "GPL-3.0",
+	"keywords": [
+		"lisk",
+		"blockchain"


We might want to extend this for LiskJS specifically.

willclarktech · 2018-02-12T15:27:11Z

Jenkinsfile

+			githubNotify context: 'continuous-integration/jenkins/lisk-js', description: 'The build failed.', status: 'FAILURE'
+		}
+		aborted {
+			githubNotify context: 'continuous-integration/jenkins/lisk-template', description: 'The build was aborted.', status: 'ERROR'


lisk-js not lisk-template

willclarktech · 2018-02-12T15:33:44Z

package.json

+		"lint": "eslint .",
+		"lint:fix": "npm run lint -- --fix",
+		"test": "NODE_ENV=test nyc mocha test",
+		"test:watch": "npm test -- --watch",


We were using chokidar because js-nacl broke mocha's watch functionality. This might have been fixed along the way with version upgrades, but we need to confirm. Does this still work ok?

As i reverted this should be reverted too.
As long as js-nacl only applied once to the global, mocha watch works fine.
I will contact add that change here

Ah great, thank you!

willclarktech · 2018-02-12T15:40:01Z

package.json

 		"serve:browsertest": "http-server browsertest",
-		"jenkins": "grunt jenkins --verbose",
-		"build": "grunt build",
+		"prebuild:node": "rm -r dist-node/* || mkdir dist-node | echo",


Should there also be a prebuild:browser script? I think it got lost from the Gruntfile.js.

willclarktech · 2018-02-12T15:40:50Z

package.json

+		"build:browsertest":
+			"npm run prebuild:browsertest && npm run babel:browsertest && npm run browserify:browsertest && npm run uglify:browsertest && npm run clean:browsertest",
+		"build:node": "npm run prebuild:node && npm run babel",
+		"build:browser": "npm run browserify && npm run uglify",


The problem with this is as a standalone script is it assumes the node build has already happened.

build command can be standalone, but browserify and unglify expects to be build before that, and each of them is not very useful as standalone.

Actually, now I see the alternative, I'm not sure it's better. :/

willclarktech · 2018-02-12T15:48:07Z

package.json

+		"prebuild:browsertest": "cp package.json browsertest/",
+		"build": "npm run build:node && npm run build:browser",
+		"build:browsertest":
+			"npm run prebuild:browsertest && npm run babel:browsertest && npm run browserify:browsertest && npm run uglify:browsertest && npm run clean:browsertest",


prebuild:browsertest runs automatically before this script is run so does not need to be specified here.

willclarktech · 2018-02-12T15:48:38Z

package.json

-		"test": "nyc mocha test --recursive",
-		"test:watch": "chokidar \"./(src|test)/**/*.js\" -c \"mocha test --recursive\" --initial",
+		"clean": "rm -rf ./node_modules ./dist-browser/lisk-js.js ./dist-browser/lisk-js.min.js ./dist-node",
+		"clean:browsertest": "rm -r browsertest/src browsertest/test",


Would this make sense as a postbuild:browsertest script?

willclarktech · 2018-02-12T15:52:21Z

test/mocha.opts

 --require ./test/setup.js
--compilers js:babel-register


We might still need this in case we want to run mocha on the source code without nyc.

Addressed

willclarktech · 2018-02-13T11:22:06Z

package.json

+		"babel": "babel src -d dist-node",
+		"babel:browsertest":
+			"babel src -d ./browsertest/src && BABEL_ENV=browsertest babel test --ignore test/transactions/dapp.js -d ./browsertest/test",
+		"postbuild:browsertest": "rm -r browsertest/src browsertest/test",


Would be great to have a slightly more logical/consistent order for these scripts.

toschdev · 2018-02-28T14:33:04Z

Jenkinsfile

@@ -17,27 +17,35 @@ pipeline {
 	stages {
 		stage('Install dependencies') {
 			steps {
-				sh '''
-				cp -r ~/cache/${CHANGE_TARGET:-${BRANCH_NAME:-development}}/node_modules ./ || true


What happened to this step?

Following up the template mainly.

I think we had it for a reason @fchavant - can you have a look please? :)

I think main reason is the cypress dependency, which takes quite long time.
However, I seems like not being a problem with current 1.0.0, also test should be as stateless as possible.

Okay, that would be quite cool actually. Yes, I agree to your point.

Cypress is still around.

@fchavant Ah, sorry. What I meant was not being problem with this current branch.
Install dependency step finishes in 1.5 min

I think probably 1 minute of the 1.5 minutes is alone cypress. Since we do not care so much about cypress version, maintaining the cache here would make sense to me.

If it is not critical to have cache here, I prefer not having the cache here.
For example, the version of cypress was affecting this PR, which would not be able to be detected easily if it was cached.

Oh. I was not aware this was causing problems already. In that case not cached is fine with me 👍

toschdev · 2018-02-28T14:35:44Z

docs/PULL_REQUEST_TEMPLATE.md

+* Linting passes
+* Tests pass
+* Commit messages follow the
+	[commit guidelines](CONTRIBUTING.md#git-commit-messages)


I think this would need to be a full link - it does not seem to work by mere reference to the CONTRIBUTING file.

It seems like it's working by looking at the view on this github

GitHub has some magic here. ✨

toschdev · 2018-02-28T14:39:44Z

package.json

+		"url": "https://github.com/LiskHQ/lisk-js/issues"
+	},
+	"engines": {
+		"node": "=6.12.3",


We have to fix the Buffer bug from node (or breaking changes in the returned error) on our side and support a great range of node versions (6.3 - 8 at least) asap. This will be done before the release.

toschdev · 2018-02-28T14:44:53Z

package.json

 	"scripts": {
 		"prestart": "./bin/prestart.sh",
 		"start": "./bin/start.sh",
-		"test": "nyc mocha test --recursive",
-		"test:watch": "chokidar \"./(src|test)/**/*.js\" -c \"mocha test --recursive\" --initial",
+		"clean": "rm -rf ./node_modules ./dist-browser/lisk-js.js ./dist-browser/lisk-js.min.js ./dist-node",


Where is this coming from? I cannot see it on lisk-template.
The reason why I ask is especially how did you decide what to "clean" and what to leave out? I am not entirely convinced. As in lisk-commander the clean command means resetting the settings (config file), here it would delete the whole distribution and node_modules. I think I would prefer if the distribution would get "cleaned" but not the node_modules.

this was not meant to be included in the commit.
we had another command to clean up before building, but that became another script, and this got left.

However I think it's useful to have it because when changing the branch, it doesnt rsync the node_module folder.
Therefore, if you change the dependencies (especially delete), you should test it with clean install, thats why I prefer to have command to clean every non-managed stuff.

toschdev · 2018-02-28T15:16:15Z

test/.eslintrc.json

@@ -8,7 +9,8 @@
 		"sinon": true
 	},
 	"rules": {
-		"arrow-body-style": "off"
+		"arrow-body-style": "off",
+		"no-unused-expressions": "off"


why turn this off?

vitaly-t · 2018-02-28T16:01:13Z

range of node versions (6.3 - 8 at least) asap. This will be done before the release.

Are you sure? Last I spoke to Oliver - 1 day ago, Node 8.x was out of the question till after the 1.0.0 release.

By the way, I was the one asking to move toward Node.js 8.x, because I would love to replace the current ES6 generators with ES7 async.

willclarktech · 2018-02-28T16:03:43Z

@vitaly-t It sounds like you're talking about the version range for which we support people running Lisk Core. Lisk Elements, as a library intended for use in other projects, has different requirements here.

Fixed reinstanciate on watch

This reverts commit ab8ac54.

toschdev · 2018-03-01T12:48:40Z

As discussed, let us keep the package-lock.json (as we support node 5+) to provide consistency and as recommended by npm. But let us gitignore yarn.lock because even though we like yarn we do not support it.

shuse2 self-assigned this Feb 9, 2018

shuse2 added question 🏗️ in progress and removed question labels Feb 9, 2018

shuse2 force-pushed the 552-update-settings branch from eb1f62a to 5b1ccf3 Compare February 9, 2018 19:50

AndrewBarba reviewed Feb 10, 2018

View reviewed changes

willclarktech reviewed Feb 12, 2018

View reviewed changes

willclarktech previously requested changes Feb 12, 2018

View reviewed changes

shuse2 added 👁️ pending review and removed 🏗️ in progress labels Feb 12, 2018

willclarktech reviewed Feb 13, 2018

View reviewed changes

shuse2 force-pushed the 552-update-settings branch 2 times, most recently from 14e257f to 10f3d37 Compare February 13, 2018 17:41

shuse2 mentioned this pull request Feb 13, 2018

Update testing module to match standard - Closes #574, #576 #559

Merged

willclarktech previously approved these changes Feb 15, 2018

View reviewed changes

shuse2 dismissed willclarktech’s stale review via 54874c2 February 16, 2018 20:59

shuse2 force-pushed the 552-update-settings branch from 046e84f to 54874c2 Compare February 16, 2018 20:59

toschdev mentioned this pull request Feb 22, 2018

Apply lisk-template #552

Closed

12 tasks

shuse2 changed the title ~~Apply template - Closes #552~~ Apply template - Closes #568,#569,#570,#571,#572,#573,#575,#578 Feb 22, 2018

shuse2 changed the title ~~Apply template - Closes #568,#569,#570,#571,#572,#573,#575,#578~~ Apply template - Closes #568,#569,#570,#571,#572,#573,#575 Feb 23, 2018

toschdev reviewed Feb 28, 2018

View reviewed changes

shuse2 force-pushed the 552-update-settings branch 2 times, most recently from bfa089a to e1c4014 Compare March 1, 2018 09:08

shuse2 and others added 13 commits March 1, 2018 10:11

♻️ Update npm scripts and setting, move docs to docs folder

3ac31d1

🐛 Fix coveralls path

d549ef5

🆕 Update testing to use chai

2e31170

Fixed reinstanciate on watch

🔥 remove lock files for npm and yarn

5663061

🔙 Revert "Updated testing to use chai"

254397d

This reverts commit ab8ac54.

♻️ Update build script

6fa1509

♻️ move eslint mocha setting to test

4d22a51

♻️ cleaned package script order and ignore comment

890bd3c

🐛 fix build script

142f386

🐛 Fix build bugs for browser

8c531ef

⬆️ Upgrade browserify to latest version

159ef81

♻️ Delete unneeded script and eslint rule

1de6dac

🔙 Revert .gitignore to include lock files

fd08708

shuse2 force-pushed the 552-update-settings branch from e1c4014 to fd08708 Compare March 1, 2018 09:12

💅 Update to ignore cypress plugin eslint

4aa8e1e

💅 Add yarn.lock file in .gitignore

8bb5968

toschdev approved these changes Mar 1, 2018

View reviewed changes

toschdev added ✅ ready and removed 👁️ pending review labels Mar 1, 2018

toschdev merged commit b398bee into 1.0.0 Mar 1, 2018

toschdev deleted the 552-update-settings branch March 1, 2018 15:49

Apply template - Closes #568,#569,#570,#571,#572,#573,#575 #558

Apply template - Closes #568,#569,#570,#571,#572,#573,#575 #558

Conversation

shuse2 commented Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewBarba Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willclarktech Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaly-t commented Feb 28, 2018 • edited Loading

willclarktech commented Feb 28, 2018 • edited Loading

toschdev commented Mar 1, 2018

shuse2 commented Feb 9, 2018 •

edited

Loading

AndrewBarba Feb 13, 2018 •

edited

Loading

willclarktech Feb 13, 2018 •

edited

Loading

vitaly-t commented Feb 28, 2018 •

edited

Loading

willclarktech commented Feb 28, 2018 •

edited

Loading