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.

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

TiAPI: Create Node-compatible util module API #10718

Merged
merged 29 commits into from Mar 7, 2019

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Feb 21, 2019

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

Description:

  • This does not includes every method off of util.types (which is new as of Node 10+).
  • This does not include util._extends()
  • This does not yet include util.isDeepStrictEqual(), which would be done after TiAPI: Create Node-compatible assert module API #10661 was also merged.
  • I did not attempt to handle util.inspect options too far. We support depth and showHidden, and to some degree breakLength; I didn't handle showProxy, compact, maxArrayLength, colors, customInspect, sorted custom functions (just Boolean). I also assume there's lots of edge cases like WeakMaps, WeakSets, Map/Set iterators, etc that we don't properly support yet.

Note that due to the way iOS proxies are set up, I can't verify the format of util.log/error/debug/puts/print messages because we cannot override console.log or console.error.

#10069 would fix that issue.

@build
Copy link
Contributor

build commented Feb 21, 2019

Warnings
⚠️

common/Resources/ti.internal/extensions/util.js#L532 - common/Resources/ti.internal/extensions/util.js line 532 – 'code' is defined but never used. (no-unused-vars)

⚠️

tests/Resources/util.addontest.js#L17 - tests/Resources/util.addontest.js line 17 – Unexpected exclusive mocha test. (mocha/no-exclusive-tests)

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 344 tests are passing.

Generated by 🚫 dangerJS against 2f7c44c

+ ' [prototype]: func { [constructor]: [Circular] } } }');
});

it.skip('with nested object', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

+ ' [length]: 1 ] }');
});

it.skip('with same object twice', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

debug: string => console.error(`DEBUG: ${string}`)
};

util.isBuffer = () => false; // FIXME: Check for Ti.Buffer? for node/browserify buffer?
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a Buffer interface around our Ti.Buffer?

util.isRegexp = value => util.types.isRegexp(value);

// FIXME: Our String.format is not very forgiving. It sort-of is supposed to do the same thing, but blows up easily
// util.format = String.format;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be removed? since you wrote your own format function further down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it was a reminder as I tried to use our String.format as a placeholder and it broke right away.

return eventListeners.length !== 0;
},
eventNames: () => Object.getOwnPropertyNames(listeners),
emitWarning: function (warning, options, code, ctor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • ⚠️ common/Resources/ti.internal/extensions/process.js line 77 – 'ctor' is defined but never used. (no-unused-vars)

});
// TODO: don't do deeper objects like this until we support the breakLength crap?

it.skip('with object', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

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

Added two notes about todo/fixes which sound rather trivial and could be done right now.

Apart from that this already looks pretty damn solid for simple shimming purposes. Nice one!

common/Resources/ti.internal/extensions/util.js Outdated Show resolved Hide resolved
common/Resources/ti.internal/extensions/util.js Outdated Show resolved Hide resolved
return Promise.reject(null);
}
const callbackified = util.callbackify(original);
callbackified((err, result) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • ⚠️ tests/Resources/util.addontest.js line 881 – 'result' is defined but never used. (no-unused-vars)

expected);
});

it.skip('with nested object and default depth', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@sgtcoolguy sgtcoolguy merged commit 60e97ce into tidev:master Mar 7, 2019
@sgtcoolguy sgtcoolguy deleted the node-util branch March 7, 2019 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants