feat(buffer): add Node.js compatible Buffer module #11084

sgtcoolguy · 2019-07-26T18:20:43Z

JIRA: https://jira.appcelerator.org/browse/TIMOB-26573

Description:
This adds a Node-compatible buffer module in JS.

In Node, a Buffer represents a byte array (specifically an unsigned byte array). In Titanium, Ti.Buffer represents the exact same concept. In both cases these APIs were created because JS engines did not yet have the concept, but have long ago added TypedArrays (or more specifically in this case, a Uint8Array).

This shim is intended to expose Node's API, which is backed by a Ti.Buffer internally. It's important to note that this could result in performance issues as a result, because to support array style index setter/getters I also had to wrap instances in a JS Proxy. So to read/write specific bytes you end up going through a JS Proxy, then traversing our JS/native binding, grabbing the value natively, returning it across the JS/native binding layer and then back out the JS Proxy. So clearly on a per-byte level this isn't ideal. But obviously a number of operations are pure JS, and there are methods that support a little bit of "bulk" inserts to help avoid the penalties.

I think long term we should investigate possibly doing a few things:

When not explicitly wrapping an existing Ti.Buffer, use a faster implementation that is backed by Uint8Array a la Browserify: https://github.com/feross/buffer/blob/master/index.js
Support accepting Uint8Array for any Titanium APIs that take in a Ti.Buffer
Possibly deprecate Ti.Buffer afterwards

Caveats
This does not include:

.buffer property
#lastIndexOf()
#read/writeBigInt method variants (we do not yet support BigInt)
#transcode()
.INSPECT_MAX_BYTES
.kMaxLength
.constants.MAX_LENGTH
.constants.MAX_STRING_LENGTH

The last 5 methods/properties are special in that they're exposed by the module returned by require('buffer') but not buffer instances or the global Buffer object.

I'm sure there are also other edge cases creating a Buffer from an ArrayBuffer/SharedArrayBuffer/Uint8Array that do not yet work.

Fixes TIMOB-26573

build · 2019-07-26T18:48:11Z

	Fails
🚫	Tests have failed, see below for more information.

	Warnings
⚠️	Commit 9500dc55a76d9cb54fca2f0f250c185f48c5b059 has a message "Update common/Resources/ti.internal/extensions/node/buffer.js Co-Authored-By: Jan Vennemann jan.vennemann@gmx.net" giving 2 errors: subject may not be empty type may not be empty
⚠️	Commit dbd07bbff551e658657590fa55ade6b26dbab64b has a message "Update common/Resources/ti.internal/extensions/node/buffer.js Co-Authored-By: Jan Vennemann jan.vennemann@gmx.net" giving 2 errors: subject may not be empty type may not be empty
⚠️	Commit d90810046f821134e65bf033ecf27b7fe1ea40b4 has a message "Update common/Resources/ti.internal/extensions/node/buffer.js Co-Authored-By: Jan Vennemann jan.vennemann@gmx.net" giving 2 errors: subject may not be empty type may not be empty

	Messages
📖	💾 Here's the generated SDK zipfile.
📖	❌ 1 tests have failed There are 1 tests failing and 472 skipped out of 4328 total tests.
📖	🚨 This PR has one or more commits with warnings/errors for commit messages not matching our configuration. You may want to squash merge this PR and edit the message to match our conventions, or ask the original developer to modify their history.

Tests:

Classname	Name	Time	Error
android.emulator.Titanium.Network.HTTPClient	clearCookieUnaffectedCheck	60.004	Error: timeout of 60000ms exceeded at Titanium.<anonymous> (/ti-mocha.js:4326:18)

Generated by 🚫 dangerJS against cdf8506

janvennemann

Did a first pass and this looks really solid! Just a few minor notes.

I'll continue to test this in my WebSocket/socket.io module.

common/Resources/ti.internal/extensions/node/buffer.js

janvennemann · 2019-07-30T10:36:03Z

common/Resources/ti.internal/extensions/node/buffer.js

+				const fillChar = bufToFillWith._tiBuffer[i % fillBufLength];
+				console.log(`going to use ${fillChar} to fill at index ${i + offset}`);
+				this._tiBuffer[i + offset] = fillChar;
+				console.log(`here's what it got set to: ${this._tiBuffer[i + offset]}`);


Remove debug logs

common/Resources/ti.internal/extensions/node/buffer.js

janvennemann · 2019-07-30T13:07:14Z

common/Resources/ti.internal/extensions/node/buffer.js

+		} else if (encoding === 'hex') {
+			return Buffer.from(stringToHexBytes(value));
+		}
+		const type = getTiCodecCharset(encoding);


Throw an uknown encoding error here if the codec map returns undefined.

janvennemann · 2019-07-30T13:09:26Z

common/Resources/ti.internal/extensions/node/buffer.js

+			return newBuffer(value);
+		}
+	}
+	throw new TypeError('The \'value\' argument must be of type: \'string\', \'Array\', \'Buffer\'');


Replace with

https://github.com/appcelerator/titanium_mobile/blob/98fa81bb2d947b4c3010a73645e5f414da7d9605/common/Resources/ti.internal/extensions/node/_errors.js#L8

That method checks using typeof versus a single type. So I can't use it here...

Ah, damn! For some reason i thought that function includes more type checks than it actually does.

janvennemann · 2019-07-31T13:59:22Z

tests/Resources/buffer.addontest.js

+
+		it('handles 2-byte utf-8 character fill', () => {
+			const b = Buffer.allocUnsafe(3).fill('\u0222');
+			should(b[0]).eql(0xc8); // FIXME: get expected undefined to equal 200


does this still throw? Do we need to skip this and the following test?

Outdated FIXME. I removed this and the others in this file

janvennemann · 2019-07-31T13:59:50Z

tests/Resources/buffer.addontest.js

+			should(buf.readUInt16BE(1).toString(16)).eql('3456');
+		});
+
+		// it('returns signed value coerced to unsigned', () => {


Could you add a note why this is commented out?

I got lazy and didn't want to write the test properly :)

janvennemann · 2019-07-31T14:01:54Z

tests/Resources/buffer.addontest.js

+			}, RangeError);
+		});
+
+		// it('throws when trying to write values out of range', () => {


Same here, can you add a fixme note and explain why this is commented out?

I'll add it back and get it right. I got a bit fatigued of writing all these tests!

Co-Authored-By: Jan Vennemann <jan.vennemann@gmx.net>

sgtcoolguy · 2019-07-31T19:02:57Z

@janvennemann Ok, I pushed changes to address the review comments.

janvennemann · 2019-08-06T11:25:25Z

common/Resources/ti.internal/extensions/node/buffer.js

+
+Buffer.poolSize = 8192;
+
+export default Buffer;


The export is not compatible with the node buffer module this way. The core module exports it as Buffer instead of default.

Some third-party node modules do the following for example:

var Buffer = require('buffer').Buffer;

good catch, pushing commits to fix this now

addressed feedback

sgtcoolguy · 2019-08-06T15:09:40Z

squash merged manually

feat(buffer): add Node.js compatible Buffer module

45bc7ea

Fixes TIMOB-26573

sgtcoolguy added ios android feature windows labels Jul 26, 2019

sgtcoolguy added this to the 8.2.0 milestone Jul 26, 2019

sgtcoolguy requested a review from janvennemann July 26, 2019 18:20

sgtcoolguy mentioned this pull request Jul 31, 2019

feat(node): add string_decoder module #11095

Closed

janvennemann requested changes Jul 31, 2019

View reviewed changes

sgtcoolguy and others added 4 commits July 31, 2019 14:19

Update common/Resources/ti.internal/extensions/node/buffer.js

9500dc5

Co-Authored-By: Jan Vennemann <jan.vennemann@gmx.net>

Update common/Resources/ti.internal/extensions/node/buffer.js

dbd07bb

Co-Authored-By: Jan Vennemann <jan.vennemann@gmx.net>

Update common/Resources/ti.internal/extensions/node/buffer.js

d908100

Co-Authored-By: Jan Vennemann <jan.vennemann@gmx.net>

refactor(buffer): address review comments

4aad66f

sgtcoolguy added 2 commits August 2, 2019 11:24

Merge branch 'master' into node-Buffer

f1e9b46

fix(buffer): fix reading of 32-bit uints

5a37d55

janvennemann previously requested changes Aug 6, 2019

View reviewed changes

sgtcoolguy added 2 commits August 6, 2019 10:37

fix(buffer): fix how buffer exports are done

a867e87

fix(buffer): fix how Buffer is hooked to global

cdf8506

sgtcoolguy closed this Aug 6, 2019

sgtcoolguy deleted the node-Buffer branch August 6, 2019 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(buffer): add Node.js compatible Buffer module #11084

feat(buffer): add Node.js compatible Buffer module #11084

sgtcoolguy commented Jul 26, 2019

build commented Jul 26, 2019 •

edited

janvennemann left a comment

janvennemann Jul 30, 2019

janvennemann Jul 30, 2019

sgtcoolguy Jul 31, 2019

janvennemann Jul 30, 2019

sgtcoolguy Jul 31, 2019

janvennemann Aug 5, 2019

janvennemann Jul 31, 2019

sgtcoolguy Jul 31, 2019

janvennemann Jul 31, 2019

sgtcoolguy Jul 31, 2019

janvennemann Jul 31, 2019

sgtcoolguy Jul 31, 2019

sgtcoolguy commented Jul 31, 2019

janvennemann Aug 6, 2019

sgtcoolguy Aug 6, 2019

sgtcoolguy commented Aug 6, 2019

feat(buffer): add Node.js compatible Buffer module #11084

feat(buffer): add Node.js compatible Buffer module #11084

Conversation

sgtcoolguy commented Jul 26, 2019

build commented Jul 26, 2019 • edited

Tests:

janvennemann left a 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

sgtcoolguy commented Jul 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgtcoolguy commented Aug 6, 2019

build commented Jul 26, 2019 •

edited