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

feat: extend global console API to be more Node-compatible #11425

Merged
merged 9 commits into from Mar 6, 2020

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Jan 9, 2020

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

Description:
This moves the global.console definition entirely into the common JS-layer so that it is defined in a cross-platform way. The removes the JS definition baked into Android's runtime JS, and removes the native Obj-c console on iOS.

Our global console object receives the Ti.API module (so that we can call info, warn, error and debug on it). Note that native logging and Ti.API have the concept of log levels, whereas node console really treats log/info/debug as just going to stdout and warn/error as going to stderr. So we have to try and retain the log level notion when possible (we sniff for being constructed with an object that has a property apiName === 'Ti.API', which allows "hijacking" the actual underlying writes in JS tests to verify the strings passed along). It also supports NodeJs stream-like objects in the constructor (but then loses the log level concept - instead passing log/info/debug to stdout and warn/error to stderr)

Note that we don't have any real shim API for NodeJS streams yet, though (so I couldn't say test a console that actually wrapped a file stream to write to files).

Oh, an important thing to note as well: I did not try to implement console.table(). It seemed... extensive.

@build
Copy link
Contributor

build commented Jan 9, 2020

Fails
🚫 Tests have failed, see below for more information.
Warnings
⚠️ This PR has milestone set to 9.1.0, but the version defined in package.json is 9.0.0 Please either: - Update the milestone on the PR - Update the version in package.json - Hold the PR to be merged later after a release and version bump on this branch
Messages
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 2 tests have failed There are 2 tests failing and 699 skipped out of 7324 total tests.

Tests:

ClassnameNameTimeError
android.emulator.Titanium.UI.Android.ProgressIndicatordialog indeterminant - show in different windows (9)5.531
Error: timeout of 5000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53723)
android.emulator.Titanium.UI.TableViewresize row with Ti.UI.SIZE on content height change (9)5.011
Error: timeout of 5000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53723)

Generated by 🚫 dangerJS against 7c85ad8

@sgtcoolguy sgtcoolguy marked this pull request as ready for review January 9, 2020 21:24
@sgtcoolguy sgtcoolguy requested a review from a team January 9, 2020 21:24
@sgtcoolguy sgtcoolguy modified the milestones: 9.0.0, 9.1.0 Jan 9, 2020
if (options && options.apiName === 'Ti.API') {
this._apiModule = options;
} else {
// if (!options || typeof options.write === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to spit out a warning / error here since this will practically create a broken console object which will lead to a crash when used due to _apiModule being undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to push my last 2 commits which implemented the stream codepath. Oops! Anyways, please re-check.

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.

Just two notes about TODO items that i think can be quickly added.

common/Resources/ti.internal/extensions/js/console.js Outdated Show resolved Hide resolved
common/Resources/ti.internal/extensions/js/console.js Outdated Show resolved Hide resolved
@sgtcoolguy sgtcoolguy merged commit e398a10 into tidev:master Mar 6, 2020
@sgtcoolguy sgtcoolguy deleted the TIMOB-26572 branch March 6, 2020 16:35
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