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

allow platform from manifest to overwrite platform #168

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

brentonhouse
Copy link
Contributor

This change allows for the platform property in the manifest to override anything that was passed in from package.json.

The reason for this is the current code does not allow the native module to exist in a sub-directory of the package. This can conflict with Alloy code with gives meaning to the ios and android directories. If those directories are needed for the JavaScript files, there will be a conflict.

With the code change, you can now specify a package.json with a titanium property like this:

  "titanium": {
	  "type": "native-module",
	  "platform": ["modules/ios", "modules/android"]
  }

where the native modules exist in a sub-directory named modules.

@brentonhouse brentonhouse requested a review from sgtcoolguy June 27, 2019 19:20
@build
Copy link

build commented Jun 27, 2019

Fails
🚫 Tests have failed, see below for more information.
Messages
📖 ❌ 2 tests have failed There are 2 tests failing and 2 skipped out of 219 total tests.

Tests:

ClassnameNameTimeError
jdk #detect()should return valid result without specifying a config or options5.079
Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/build/jenkins/workspace/cli_node-appc_PR-168/test/test-jdk.js)
timodule #detectNodeModules()detects cross-platform native module with platform-specific manifests0.002
expected Array [ 'iphone' ] to equal Array [ 'ios' ] (at '0', A has 'iphone' and B has 'ios')
[
+  "ios"
-  "iphone"
 ]

Generated by 🚫 dangerJS against cbbfb02

@sgtcoolguy
Copy link
Contributor

I still fail to understand why this is necessary. Why would Alloy be looking inside the npm packages holding native modules? If you're looking to copy node_modules into the app, any npm packages with the special titanium.type === 'native-module' package.json value should be ignored.

@sgtcoolguy
Copy link
Contributor

I believe the tests may be failing because I think the platform values are meaningful - they expect our typical naming schemes used for platform specific contents: ios, iphone, androidorwindows`

@brentonhouse
Copy link
Contributor Author

I need to package JavaScript code inside the same package as the native modules. If I have to put the JavaScript in one package and the native code in the other and make the one with the native dependent on the one with the JavaScript, I don't think it will be found by the current logic because it only looks in the root node_modules directory.

Plus, having all the code you need in one package is much more convenient and easier to manage and install. Sometimes the native API for an iOS module is different than the API for an Android module. The JavaScript files can wrap them and help make them appear to be the same and save developers a lot of if(OS_IOS) else if(OS_ANDROID) type code.

@cb1kenobi
Copy link
Contributor

I'm OK with a single npm package containing multiple platform-specific modules, hence the subdirectories.

I'm not OK with the modules/ prefix in the "platform".

We should completely ignore the platform in the manifest. We don't need it. The manifest should go away.

Instead, if the "platform" in the package.json is a string, we can assume the root of the package is the module for the specified platform.

If the "platform" is an array, then we can assume the module(s) live in subdirectories matching the platform name.

I have no idea why "platform" would ever be an object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants