Add multiple node.js version support - Closes #561 by shuse2 · Pull Request #562 · LiskArchive/lisk-elements

shuse2 · 2018-02-14T21:32:52Z

Closes #561

Description

Changed hexToBuffer behavior to mimic node version >= 8 in all versions.
Changed Buffer.from to use hexToBuffer

Note

Run test against

v6.12.3
v7.10.1
v8.9.4
v9.5.0

Changes

Update all direct usage of Buffer.from(hex, 'hex')
Buffer.from(hex, 'hex') should throw Type error if first argument is not string
Buffer.from(hex, 'hex') should ignore invalid hex string

AndrewBarba · 2018-02-14T21:38:32Z

 	"main": "./dist-node/index.js",
+	"engines": {
+		"node": ">=6 <=9",
+		"npm": "=>3 <=5"


Small typo =>3 should be >=3

willclarktech

Tests fail on Node.js v6.0.0. I couldn't complete installation on Node.js v8.0.0.

Found some remaining Buffer.froms which should be updated in src:

▶ grep -R -n Buffer.from src
src/crypto/convert.js:37:	return Buffer.from(matchedHex.slice(0, evenLength).join(''), 'hex');
...
src/transactions/utils/getTransactionBytes.js:184:		? Buffer.from(transaction.signature, 'hex')

Additionally there are Buffer.from(xxx, 'hex') cases in test. Maybe we should also be using our function there?

willclarktech · 2018-02-15T10:29:21Z

 export const bufferToHex = buffer => naclInstance.to_hex(buffer);

-export const hexToBuffer = hex => Buffer.from(hex, 'hex');
+const hexRegex = /([0-9]|[a-f])/gim;


Do we really want multiline?

This regex gives odd behaviour.

> Buffer.from('abhcd', 'hex') // Node 6: throws 'Invalid hex string' <Buffer ab> // Node 8 > hexToBuffer('abhcd') <Buffer abcd>

will be changed to take only first part =)

willclarktech · 2018-02-15T10:35:12Z

+		it('should create even numbered Buffer from odd number hex string', () => {
+			const buffer = hexToBuffer('c3a5c3a4c3b6a');
+			return buffer.should.be.eql(Buffer.from('c3a5c3a4c3b6', 'hex'));
+		});


We need a test case where there's a non-hex character in the middle of hex characters.

willclarktech · 2018-02-15T10:36:04Z

 			const buffer = hexToBuffer(defaultHex);
 			return buffer.should.be.eql(defaultBuffer);
 		});
+		it('should create empty Buffer from non-string', () => {


Should it? Node 6 and 8 both throw errors if you use a number for example.

Will be changed to throw type error if it's not string

shuse2 · 2018-02-15T13:52:45Z

I think the Buffer.from used in the test is the controlled variable, which shouldn't throw error, so not really necessary to change there

shuse2 · 2018-02-15T15:10:05Z

Until node v6.3.0, createCipheriv with "aes-256-gcm" did not allow IV to be longer than 12 bytes.
but changed in
nodejs/node#6376
In our code, we use 16 bytes, so changed engine to start from 6.3

addressed

willclarktech · 2018-02-15T15:31:46Z

Whoops, sorry just realised one of my Buffer.from finds in src was for the hexToBuffer function which obviously needs to remain.

willclarktech

We still need to consider npm. I get an installation failure with npm v5.0.0.

willclarktech · 2018-02-15T15:16:27Z

 export const bufferToHex = buffer => naclInstance.to_hex(buffer);

-export const hexToBuffer = hex => Buffer.from(hex, 'hex');
+const hexRegex = /^([0-9]|[a-f])+/gi;


Let's make this regex as simple as possible. Does the following work?

const hexRegex = /^[0-9a-f]+/i;

i think global key is not needed but ^([0-9]|[a-f]) this is more readable isn't it?

As discussed, a capturing group which never gets used is confusing.

willclarktech · 2018-02-15T15:19:24Z

 	"description": "JavaScript library for sending Lisk transactions from the client or server",
 	"main": "./dist-node/index.js",
+	"engines": {
+		"node": ">=6.3 <=9",


We should put an upper bound on the latest version of 9 that we've tested.

willclarktech · 2018-02-15T15:26:20Z

+const hexRegex = /^([0-9]|[a-f])+/gi;
+export const hexToBuffer = hex => {
+	if (typeof hex !== 'string') {
+		throw new TypeError('argument must be string');


Argument must be a string

willclarktech · 2018-02-15T15:27:48Z

+		throw new TypeError('argument must be string');
+	}
+	const matchedHex = hex.match(hexRegex) || [];
+	if (matchedHex.length === 0) {


Why not just this?

const matchedHex = hex.match(hexRegex); if (!matchedHex) { return Buffer.alloc(0); }

In the following code, it uses first element like matchedHex[0], so I think it would be better to check length here already

willclarktech · 2018-02-15T15:33:52Z

+		it('should create even numbered Buffer from odd number hex string', () => {
+			const buffer = hexToBuffer('c3a5c3a4c3b6a');
+			return buffer.should.be.eql(Buffer.from('c3a5c3a4c3b6', 'hex'));
+		});


Would also be good to have a test case with a valid odd-numbered hex string followed by non-hex. Eg. 123xxx and/or 123x.

Addressed =)

shuse2 · 2018-02-16T08:40:12Z

I get an installation failure with npm v5.0.0.

this was solved by install without cache.

willclarktech · 2018-02-22T09:41:16Z

 			return buffer.should.be.eql(defaultBuffer);
 		});
+		it('should throw TypeError with number', () => {
+			return (() => hexToBuffer(123)).should.throw(


Prefer hexToBuffer.bind(null, 123).should.throw syntax.

willclarktech · 2018-02-22T09:42:44Z

 			const buffer = hexToBuffer(defaultHex);
 			return buffer.should.be.eql(defaultBuffer);
 		});
+		it('should throw TypeError with number', () => {


Prefer an empty line between its.

willclarktech · 2018-02-22T09:43:46Z

 		});
+		it('should throw TypeError with number', () => {
+			return (() => hexToBuffer(123)).should.throw(
+				new TypeError('argument must be string'),


This test gives false positives: the message is wrong here.

Addressed

willclarktech

Would like some more extensive testing on different versions, but this looks good to me. 👍

toschdev · 2018-02-28T14:06:29Z

 export const bufferToHex = buffer => naclInstance.to_hex(buffer);

-export const hexToBuffer = hex => Buffer.from(hex, 'hex');
+const hexRegex = /^[0-9a-f]+/i;


toschdev · 2018-02-28T14:26:07Z

+		return Buffer.alloc(0);
+	}
+	const evenLength = Math.floor(matchedHex.length / 2) * 2;
+	return Buffer.from(matchedHex.slice(0, evenLength), 'hex');


Do you think it is better to return a sliced element here rather than throwing an error in case it is not even?

Same applies to non-hex strings

I think main reason not to throw an error for non-even input is because node 8 is not behaving like that.

Also, for the non-hex string, node 8 ignores the non hex string, and create the buffer from whatever it can from the front.

> process.version 'v8.9.4' > Buffer.from('abcde', 'hex'); <Buffer ab cd> > Buffer.from('abcdzz123', 'hex'); <Buffer ab cd>

But this is our opportunity to make the behaviour we actually want.

nodejs/node#12012 (comment)
I think main thing is that parallel behavior with base64 and coherent eror handlings of various things it can happen

Actually the sentiment seems to be like "you can do validation on your side if you want" on the node side of things: nodejs/node#8569

And in my opinion I would prefer to have an error thrown.

Addressed

shuse2 added the 👁️ pending review label Feb 14, 2018

shuse2 self-assigned this Feb 14, 2018

shuse2 requested review from toschdev and willclarktech February 14, 2018 21:32

AndrewBarba reviewed Feb 14, 2018

View reviewed changes

willclarktech previously requested changes Feb 15, 2018

View reviewed changes

willclarktech added 🏗️ in progress and removed 👁️ pending review labels Feb 15, 2018

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

willclarktech previously requested changes Feb 15, 2018

View reviewed changes

willclarktech previously approved these changes Feb 16, 2018

View reviewed changes

shuse2 dismissed willclarktech’s stale review via fee66fb February 16, 2018 17:01

shuse2 force-pushed the 561-support_multiple_node_version branch 2 times, most recently from fee66fb to 11e2f25 Compare February 16, 2018 17:04

willclarktech previously requested changes Feb 22, 2018

View reviewed changes

willclarktech previously approved these changes Feb 22, 2018

View reviewed changes

toschdev previously requested changes Feb 28, 2018

View reviewed changes

toschdev added 🏗️ in progress and removed 👁️ pending review labels Feb 28, 2018

shuse2 dismissed willclarktech’s stale review via f919188 February 28, 2018 17:24

shuse2 force-pushed the 561-support_multiple_node_version branch from f919188 to 6a41d85 Compare February 28, 2018 17:27

🌱 Add multiple node.js version support

17c4338

shuse2 and others added 7 commits February 28, 2018 18:28

🐛 Fix typo on package.json

d1ef215

🐛 Fix hex buffer behavior to match node 8

6448d9f

⬆️ Update package.json engine to support from node 6.3

ff40ba4

♻️ Update regex and check logic

a6a5e07

🏠 Fix rebase error

2dcd2d2

♻️ Fix test format and message check

3a3d646

🌱 Update hexToBuffer behavior to throw error when invalid

01f257b

shuse2 force-pushed the 561-support_multiple_node_version branch from 6a41d85 to 01f257b Compare February 28, 2018 17:29

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

toschdev approved these changes Mar 1, 2018

View reviewed changes

toschdev merged commit 794a7d4 into 1.0.0 Mar 1, 2018

toschdev deleted the 561-support_multiple_node_version branch March 1, 2018 08:32

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

Conversation

shuse2 commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Note

Changes

Uh oh!

AndrewBarba Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willclarktech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shuse2 commented Feb 15, 2018

Uh oh!

shuse2 commented Feb 15, 2018

Uh oh!

willclarktech commented Feb 15, 2018

Uh oh!

willclarktech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shuse2 commented Feb 16, 2018 • edited by willclarktech Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willclarktech left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shuse2 commented Feb 14, 2018 •

edited

Loading

AndrewBarba Feb 14, 2018 •

edited

Loading

shuse2 commented Feb 16, 2018 •

edited by willclarktech

Loading

willclarktech left a comment •

edited

Loading