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

Add the reject function at the expected errorIndex position in the args array #436

Merged
merged 1 commit into from
Aug 17, 2016
Merged

Conversation

kevinboosten
Copy link
Contributor

@kevinboosten kevinboosten commented Aug 16, 2016

We don't want that the reject cb takes the position of an optional argument that has not been defined.
For example the Dialogs.alert method takes an optional 'buttonLabel' string. In case we do not set this value, and thus want to use the default value, the 'reject' callback get spliced into this position due the fact that the splice start index is bigger than the array length.

Dialogs.alert('mytitle', 'mymessage', 'Okay')
args will become:
["title", resolve, "message", "My button text", reject]

Dialogs.alert("title", "message")
args will become:
["title", resolve, "message", reject]

The latter will invoke the cordova-plugins-dialog alert method with the wrong arguments
--> reject is on the position of the buttonLabel

cordova-plugins-dialog alert method signature
alert: function(message, completeCallback, title, buttonLabel) {..}

…gs array

We don't want that the reject cb takes the position of an optional
argument that has not been defined
For example the Dialogs.alert method takes an optional 'buttonLabel'
string. In case we do not set this value, and thus want to use the
default value, the 'reject'
callback get spliced into this position due the fact that the splice
start index is bigger than the array length.
Dialogs.alert("title", "message", "My button text") --> args =
["title",  resolve, "message", "My button text", reject]
Dialogs.alert("title", "message") -->  args = ["title", resolve,
"message", reject] --> reject is on the position of the buttontitle!

The cordova-plugin-dialogs alert function receives the wrong arguments
—> alert: function(message, completeCallback, title, buttonLabel)
The buttonLabel will receive the "reject" callback instead of a
undefined value.
@ihadeed
Copy link
Collaborator

ihadeed commented Aug 17, 2016

This is already taken into consideration when it comes to the dialogs plugin:
image

However, we can still look into merging this as it might help solve future conflicts.

@kevinboosten
Copy link
Contributor Author

Hi @ihadeed,

You mean the specific success/error Index? I'm aware of that, but it's not guaranteed that the "errorIndex" also takes that specific index in the args array.

I encountered this problem in combination with angular 1 and the ionic-native-bower package, but it also occurs in an Ionic 2 app obviously.

@ihadeed
Copy link
Collaborator

ihadeed commented Aug 17, 2016

No I meant the default values that are set to the parameters.

I expected them to be passed to the plugin if the developer hasn't passed any values. But it turned out it doesn't work that way, since the decorators override that functionality. This is what it looks like in ES5:
image

@kevinboosten
Copy link
Contributor Author

Ah, and eventually it will fallback on the cordova-plugin-dialogs implementation default values:

alert: function(message, completeCallback, title, buttonLabel) { var _title = (typeof title === "string" ? title : "Alert"); var _buttonLabel = (buttonLabel || "OK"); exec(completeCallback, null, "Notification", "alert", [message, _title, _buttonLabel]); }

where in this case the fourth argument is not a string but it's defined:
["title", resolve, "message", reject]

and will not set the _buttonLabel to "OK" and invokes the exec command with a wrong value.

So your're saying the root cause to this problem is actually something else? I'll try to look into it.

@ihadeed ihadeed merged commit 4e87ac7 into danielsogl:master Aug 17, 2016
@ihadeed
Copy link
Collaborator

ihadeed commented Aug 17, 2016

No, the solution you provided works well. It solves the problem.

I think we need to do the same thing for successIndex.

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.

None yet

2 participants