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

[TIMOB-26080] Fix ERR_INVALID_CALLBACK error on Android build and misc Node 10 fixes #10090

Merged
merged 3 commits into from Jun 12, 2018

Conversation

ewanharris
Copy link
Collaborator

This PR contains two tickets as they are closely related, however the second commit has some consequences on our supported Node version, as Buffer.from is only available in Node 6+. We can easily ponyfill this using buffer-from but I chose to drop Node 4 for the following reasons

That said, if anyone has differing opinions, I'm happy to rework fd207d0 to use the ponyfill

Tickets:

  1. JIRA: https://jira.appcelerator.org/browse/TIMOB-26080
  • Fix the ERR_INVALID_CALLBACK error when building a project with .aar files.
    • Steps to reproduce:
    1. Download the facebook module
    2. Build using this SDK and Node 10, no error should be thrown
  1. JIRA: https://jira.appcelerator.org/browse/TIMOB-26081
  • Remove deprecation warnings from new Buffer usage
  • It's kinda easy to miss these logs in a debug/trace log level, it's easy to spot in info. They look like (node:19383) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
    • Steps to reproduce:
    1. Build an app for iOS where the DefaultIcon.png has an alpha channel, here's one
    2. Build any Android module

@@ -26,7 +26,7 @@
"android platform tools": "27.x",
"android tools": "<=26.x",
"android ndk": ">=r11c <=r16c",
"node": ">=4.0 <=7.x",
"node": ">=6.0 <=8.x",
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we don't need <= 10.x here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Left out on purpose as this PR doesn't introduce full Node 10 support. This + #10089 + some further testing and I'd feel comfortable claiming support

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright. I think it should be added and tested with this ticket, as it's all about Node 10 anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Want me to cherry-pick your commit across to here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it a different change? My one only updated ioslib and it's compatibility, but I think the CLI will warn if the package.json in the iphone/android dir of the SDK are not updated as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ewanharris I've merged Hans' ioslib update PR after fixing ioslib/node-ios-device, so this can now be tested for Node 10 support.

@ewanharris ewanharris changed the title [TIMOB-26080] Fix ERR_INVALID_CALLBACK error on Android build [TIMOB-26080] Fix ERR_INVALID_CALLBACK error on Android build and misc Node 10 fixes Jun 5, 2018
@ewanharris
Copy link
Collaborator Author

Hmm it appears I'm wrong about the Buffer.from availability, docs state it was added in 5.10.0, but it does appear to exist in Node 4

@sgtcoolguy
Copy link
Contributor

Related: tidev/node-appc#129

@sgtcoolguy
Copy link
Contributor

So I think the changes to the node version range should probably be rolled back if this change still does work on Node 4, and likely the top-level package.json should be updated to use node-appc 0.24.6 - then this PR can be merged.

I think if do decide to bump our minimum Node support, it should be done in a separate PR explicitly with some attention to Brian to update the docs/support matrix.

@ewanharris ewanharris force-pushed the TIMOB-26080 branch 2 times, most recently from 33d5f18 to d485bc6 Compare June 12, 2018 13:27
@ewanharris
Copy link
Collaborator Author

Update to reinstate the node 4 compatibility with an update to latest node-appc and reverting the package.json changes

@build
Copy link
Contributor

build commented Jun 12, 2018

Fails
🚫

Tests have failed, see below for more information.

Messages
📖

💾 Here's the generated SDK zipfile.

Tests:

Classname Name Time Error
android.Titanium.UI.TabGroup add Map.View to TabGroup 10.022 Error: timeout of 10000ms exceeded

Generated by 🚫 dangerJS

@sgtcoolguy sgtcoolguy merged commit 5091225 into tidev:master Jun 12, 2018
@sgtcoolguy sgtcoolguy added this to the 7.4.0 milestone Jun 12, 2018
@hansemannn hansemannn modified the milestones: 7.4.0, 7.5.0 Aug 24, 2018
@ewanharris ewanharris deleted the TIMOB-26080 branch September 18, 2018 15:46
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

4 participants