feat(node): node 12 compatible util module and improved console #11165

janvennemann · 2019-08-22T11:35:19Z

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

Optional Description:
This refactors the util module by adding missing type checks for util.types and using the Node 12 inspect implementation. It is mostly a copy of internal/util/inspect.js and internal/util/types.js that i modified to work with Titanium. That way it should be easier for us to catch up with internal changes inside Node since we use the same code as they do, only with a few slight modifications to get around stuff that calls into Node's native code.

Additionally this hooks up util.inspect with console.log and the likes to get improved logging capabilities in the console, just like it is in Node. I left Ti.API.* untouched for now, not sure if we want to sync those up too.

Since iOS and Android run on different JS engines there're a few slight differences when it comes to inspecting errors or hidden object properties, but i think those can be neglected for now.

Other issues i discovered so far:

Error stack is different on V8/JSC. I try to normalize the stack traces so inspected errors look almost the same.
We can't peek into weak collections, so they will always print as WeakMap { <items unknown> } for example, even if showHidden option is set.
~~Our own Buffer implementation doesn't get handled properly because it is wrapped in a Proxy~~ I added a workaround for that and also added the custom inspect implementation for Node Buffers.
Proxies can't be inspected as such since it is currently not possible to detect them from within JS without special handling in the proxy itself. They will print as the object they act as the proxy for without any info about the proxy itself.

build · 2019-08-22T11:37:26Z

	Warnings
⚠️	common/Resources/ti.internal/extensions/node/buffer.js#L76 - common/Resources/ti.internal/extensions/node/buffer.js line 76 – Missing JSDoc parameter description for 'arg'. (valid-jsdoc)
⚠️	common/Resources/ti.internal/extensions/node/buffer.js#L77 - common/Resources/ti.internal/extensions/node/buffer.js line 77 – Missing JSDoc parameter description for 'encodingOrOffset'. (valid-jsdoc)
⚠️	common/Resources/ti.internal/extensions/node/buffer.js#L78 - common/Resources/ti.internal/extensions/node/buffer.js line 78 – Missing JSDoc parameter description for 'length'. (valid-jsdoc)

	Messages
📖	💾 Here's the generated SDK zipfile.
📖	✅ All tests are passing Nice one! All 3979 tests are passing. (There are 469 skipped tests not included in that total)
📖	✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Generated by 🚫 dangerJS against 0d381ce

sgtcoolguy

So I'm fine with making use of Node's code when possible if the copyright/license allows for it. I had specifically been avoiding it in my re-implementations. But the reality is that our Android core has some Node code from very, very old Node versions in it already (i.e. events.js)

I think we need to revisit if we need to be retaining special copyright headers or shipping license files/etc to not be in violation of Node (and it's upstream contributors/dependencies)'s license/usage.

sgtcoolguy · 2019-08-26T16:49:05Z

.eslintrc

  },
  "rules": {
    "strict": ["error", "global"]
  },
  "overrides": [
    {
-      "files": [ "tests/Resources/es6.*.js" ],
+      "files": [ "tests/Resources/es6.*.js", "tests/Resources/util.test.js" ],


at this point, perhaps just bite the bullet and say all test/Resources/** are 2017/module? Or even 2018/2019?

sgtcoolguy · 2019-08-26T16:49:24Z

.eslintrc

@@ -46,7 +46,7 @@
    {
      "files": [ "common/Resources/**/*.js" ],


So maybe merge this and the entry above?

I had to bump the version to 2018 for either object rest spread or async/generator support, not sure anymore.

Maybe we should even switch to axway/+babel or manually setting the parser to babel-eslint for stuff like this that gets transpiled?

sgtcoolguy · 2019-08-26T16:50:35Z

common/Resources/ti.internal/extensions/js/console.js

+const kColorInspectOptions = { colors: true };
+const kNoColorInspectOptions = {};
+
+console.debug = function (...args) {


Out of the scope of this PR, but we do have an open ticket to expand our console support. @ewanharris had added time/timeLog before, but it'd be good to start moving this all into this common area.

I was actually thinking to go one step ahead and adopt more of Nodes console API but then left it as is to not blow this out of proportion.

Is there any open ticket for the extended console functionality?

https://jira.appcelerator.org/browse/TIMOB-26572

sgtcoolguy · 2019-08-26T16:53:30Z

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

+	}
+
+	if (otherClasses.length !== 0) {
+		otherClasses.forEach((clazz) => {


I prefer to drop the wrapping parens in single-arg arrow functions...

Yeah, me too, but since this was also taken from the node source, i tried to make only the neccessary changes to make our linting pass (for example, wrap single line if-statements in brackets) and leave everything else as is.

sgtcoolguy · 2019-08-26T16:56:40Z

tests/Resources/util.test.js

+/* eslint no-array-constructor: "off" */
+/* eslint no-new-wrappers: "off" */
+
+const should = require('./utilities/assertions');


Have any hints on what actually changed here in the override test suite?

I needed to fix the typo in isRegexp in the existing tests and added tests for the missing methods hanging off of types. So all the typed array stuff at the end of this file as well as

isAnyArrayBuffer

isArrayBuffer

isArgumentsObject

isAsyncFunction

isDataView

isGeneratorFunction

isGeneratorObject

isMapIterator

isPromise

isSharedArrayBuffer

isSetIterator

isSymbolObject

isWeakMap

isWeakSet

Also some of the output generated by format changed, so i had to adjust the existing tests there too. For example, the break length was increased and additional new lines were added when printing objects.

janvennemann · 2019-08-28T16:31:19Z

@sgtcoolguy so the files in lib all have a copyright header, the ones in lib/internal don't. That's why i just added a reference to the original file via @see in the first place.

Node's LICENSE file states that the copyright notice and permission notice should be included in all copies, so i'll add that to the respective files.

build · 2019-09-10T20:02:16Z

common/Resources/ti.internal/extensions/node/internal/util/types.js

+}
+
+export function isSharedArrayBuffer(value) {
+	if (!global.SharedArrayBuffer) {


⚠️ common/Resources/ti.internal/extensions/node/internal/util/types.js line 189 – The 'SharedArrayBuffer' is not supported until Node.js 8.10.0. The configured version range is '>=8'. (node/no-unsupported-features/es-builtins)

janvennemann · 2019-09-10T20:14:18Z

@sgtcoolguy added the license header from Node.js and updated our linting setup to ignore various node rules that don't apply to our node shims.

sgtcoolguy · 2019-09-13T16:56:33Z

I don't know why, but on Android this PR seems to finish the test suite and then sit there until it gets timed out. Not sure what could be going on that would cause that. I guess I'll have to try it locally and see?

sgtcoolguy · 2019-09-13T17:55:42Z

Ok, well after some playing around locally, it's because the wrapping code that runs our tests it getting into a state where it think it never has the full output for a test failure. Specifically we're spitting out a TEST_END with the json results and it's getting truncated somehow, so we never have the full valid JSON to parse (and it keeps appending the next line of output it sees to it to try and make it valid). The error crops up in the log: https://jenkins.appcelerator.org/blue/rest/organizations/jenkins/pipelines/titanium-sdk/pipelines/titanium_mobile/branches/PR-11165/runs/16/nodes/89/steps/154/log/?start=0

at the "util#format() with object" test.

So this appears to be a bug in our actual Ti.API log implementation, that is because of a native logger limit. Android itself tries to hack around this at the Java side by splitting long messages to avoid truncation: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/util/Log.java#401

sgtcoolguy · 2019-09-13T18:16:20Z

Created a TIMOB JIRA ticket for this android Ti.API limitation: https://jira.appcelerator.org/browse/TIMOB-27399

sgtcoolguy · 2019-09-13T18:47:40Z

ok, well for now I have hacked a workaround into the test suite code. If a stringified test failure is too long, I split the JSON across multiple lines. And I handle on the other wrapping script that reads the logs and updated it to strip the [INFO] prefix on subsequent lines.

ssjsamir

FR Passed: Can see an improvement in console debugging capabilities and apps no longer crash.

Testcase:

const a = {};
a.b = a;
console.log(a); // crashes on 8.0.1.GA, or just prints "[object Object]" on 8.1.0.GA
//Expected <ref *1> { b: [Circular *1] }
 
const b = {
foo: 'bar'
}
console.log(b); // prints "{ foo: 'bar' }" on 8.0.1.GA, or just "[object Object]" on 8.1.0.GA
//Expected  { foo: 'bar' }

const map = new Map();
map.set('foo', 'bar');
console.log(map); // prints "{}" on 8.0.1.GA, or just "[object Map]" on 8.1.0.GA
//Expected Map { 'foo' => 'bar' }

Output:

[INFO] �� � <ref *1> { b: [Circular *1] }
[INFO] �� � { foo: 'bar' }
[INFO] �� � Map { 'foo' => 'bar' }

Test Environment

MacOS Catalina 10.15 Beta
Node.js ^8.11.1
"NPM":"4.2.15","CLI":"7.1.1"
Java Version 1.8.0_131
Xcode 11 GM seed 2

build · 2019-10-03T14:03:01Z

tests/Resources/util.test.js

+				util.types.isAnyArrayBuffer(ab).should.be.true;
+			});
+
+			it.skip('should return true for built-in SharedArrayBuffer', () => {


⚠️ tests/Resources/util.test.js line 1223 – Unexpected skipped mocha test. (mocha/no-skipped-tests)

feat(node): node 12 compatible util module and improved console

ee4555a

janvennemann added the improvement label Aug 22, 2019

janvennemann added this to the 8.2.0 milestone Aug 22, 2019

janvennemann requested a review from sgtcoolguy August 22, 2019 11:35

fix: fix typo and add tests for weak map/set

28ca293

janvennemann added 8 commits August 22, 2019 14:15

feat: enable color mode for inspect

7af73e9

chore: update eslint config

2d4d0c8

chore: fix linting issues

1edff08

fix: disable bigint type checks

934fd56

fix: rename isRegexp usage to isRegExp

5492f35

fix: use correct should assertion syntax

d6170c8

chore: disable lint warning

595189f

feat: add custom inspect behavior for buffers

a847183

sgtcoolguy approved these changes Aug 26, 2019

View reviewed changes

Merge branch 'master' into TIMOB-27345

78569bb

build added the in-qe-testing 🕵 label Aug 30, 2019

sgtcoolguy modified the milestones: 8.2.0, 8.3.0 Sep 5, 2019

sgtcoolguy and others added 3 commits September 6, 2019 10:38

Merge branch 'master' into TIMOB-27345

b93ec69

chore: add node license header

a6d1d89

style(eslint): adjust linting rules, fix various warnings

8193f8e

build reviewed Sep 10, 2019

View reviewed changes

chore: add node license header

9e79450

sgtcoolguy added 2 commits September 12, 2019 11:29

Merge branch 'master' into TIMOB-27345

7b1cc8d

Merge branch 'master' into TIMOB-27345

5f0038d

Merge branch 'master' into TIMOB-27345

648f1e3

sgtcoolguy removed the in-qe-testing 🕵 label Sep 18, 2019

Merge branch 'master' into TIMOB-27345

714219f

build added the in-qe-testing 🕵 label Sep 18, 2019

Merge branch 'master' into TIMOB-27345

8ed8a82

ssjsamir self-requested a review September 26, 2019 12:33

ssjsamir approved these changes Sep 26, 2019

View reviewed changes

test(util): update format tests to pass on ios and android

0d381ce

build reviewed Oct 3, 2019

View reviewed changes

sgtcoolguy merged commit 2b48840 into tidev:master Oct 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node): node 12 compatible util module and improved console #11165

feat(node): node 12 compatible util module and improved console #11165

janvennemann commented Aug 22, 2019 •

edited

build commented Aug 22, 2019 •

edited

sgtcoolguy left a comment

sgtcoolguy Aug 26, 2019

sgtcoolguy Aug 26, 2019

janvennemann Aug 28, 2019

sgtcoolguy Aug 26, 2019

janvennemann Aug 28, 2019

sgtcoolguy Aug 30, 2019

sgtcoolguy Aug 26, 2019

janvennemann Aug 28, 2019

sgtcoolguy Aug 26, 2019

janvennemann Aug 28, 2019

janvennemann commented Aug 28, 2019

build Sep 10, 2019

janvennemann commented Sep 10, 2019

sgtcoolguy commented Sep 13, 2019

sgtcoolguy commented Sep 13, 2019

sgtcoolguy commented Sep 13, 2019

sgtcoolguy commented Sep 13, 2019

ssjsamir left a comment

build Oct 3, 2019

feat(node): node 12 compatible util module and improved console #11165

feat(node): node 12 compatible util module and improved console #11165

Conversation

janvennemann commented Aug 22, 2019 • edited

build commented Aug 22, 2019 • edited

sgtcoolguy 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

janvennemann commented Aug 28, 2019

Choose a reason for hiding this comment

janvennemann commented Sep 10, 2019

sgtcoolguy commented Sep 13, 2019

sgtcoolguy commented Sep 13, 2019

sgtcoolguy commented Sep 13, 2019

sgtcoolguy commented Sep 13, 2019

ssjsamir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janvennemann commented Aug 22, 2019 •

edited

build commented Aug 22, 2019 •

edited