Allow JS init with param app Id

[podotki]

Hi !

As the title says, just add handling of the parameter app Id at the init.
Let me know if it's ok for you.
This is not a big change and I think it can help a lot the community so feel free to update the code to be cleaner as I really wish that PR to be merged ;)

Thanks a lot :)

[fbartho]

@podotki -- is this ready for review? -- It looks great already -- thanks for contributing!

[podotki]

Mmm I think there is an error as I don't know much about Objective-C neither Java...
I'm checking it ;)

[fbartho]

At your leisure @podotki please let me know if you've got it working and this is ready for review!

[fbartho]

@podotki I'm starting to think about a new release -- do you think this is ready for final review and testing?

[fbartho]

This code is available with other changes in https://www.npmjs.com/package/react-native-zendesk-chat/v/0.4.1-beta.4

[emkman]

@podotki @fbartho this breaks existing apps unfortunately. Despite the docs stating appId is optional, it is not declared that way in objective C and upgrading casues this error:

RNZendeskChatModule.init was called with 1 arguments but expects 2 arguments. If you haven't changed this method yourself, this usually means that your versions of the native code and JavaScript code are out of sync. Updating both should make this error go away.

-[RCTModuleMethod invokeWithBridge:module:arguments:]
    RCTModuleMethod.mm:549
facebook::react::invokeInner(RCTBridge*, RCTModuleData*, unsigned int, folly::dynamic const&)
facebook::react::RCTNativeModule::invoke(unsigned int, folly::dynamic&&, int)::$_0::operator()() const
invocation function for block in facebook::react::RCTNativeModule::invoke(unsigned int, folly::dynamic&&, int)
_dispatch_call_block_and_release
_dispatch_client_callout
_dispatch_lane_serial_drain
_dispatch_lane_invoke
_dispatch_workloop_worker_thread
_pthread_wqthread
start_wqthread

I had to change:
ZendeskChat.init(env.ZENDESK_ACCOUNT_KEY);
to
ZendeskChat.init(env.ZENDESK_ACCOUNT_KEY, null);
but I don't think this is the expected or desired behavior

[fbartho]

@emkman -- that's strange. I'm not getting that error in our codebase -- indeed I only saw that error on Android and fixed it in the release branch.

What version of React-Native are you using?
Are you using Hermes?

[emkman]

@fbartho "react-native": "0.63.3", on iOS 14 in the simulator. Not using Hermes.

Looking at RNZendeskChatModule.m it seems like appId is a required string param of init now. I think you could change it from NSString to id or override the method with a second definition though I am not sure how that works with RCT_EXPORT tbh.

RCT_EXPORT_METHOD(init:(NSString *)zenDeskKey appId:(NSString *)appId) {
	if (appId) {
		[ZDKChat initializeWithAccountKey:zenDeskKey appId:appId queue:dispatch_get_main_queue()];
	} else {
		[ZDKChat initializeWithAccountKey:zenDeskKey queue:dispatch_get_main_queue()];
	}
}

[fbartho]

I suspect maybe RN < 0.63.3 behaves differently, or something else was going on.

@emkman -- I don't think overrides work with RCT_EXPORT_METHOD
Regardless, if you want to fix it, I patched a similar thing for Android in this commit: 6906a29

So we could apply a similar patch for iOS.

If you're feeling up to it, in this PR I fixed a similar

[emkman]

So we could apply a similar patch for iOS.

If both OSes need this, does it make sense to keep using the Platform patch at the RN level? It might be cleaner to just implement separate init and initWithAppId methods.

[fbartho]

@emkman I agree with what you're suggesting with a slight twist:

Instead of Platform.select, just always attach an init method:

RNZendeskChatModule.init = (key, appId) => {
 	return RNZendeskChatModule._initWith2Args(key, appId || null);
};

And define _initWith2Args on iOS as well as on Android

[emkman]

@fbartho this way would mean no changes are needed for Android, correct? Works for me.