chore(cli): Add functionality class to plugin template

[jcesarmobile]

No description provided.

[jcesarmobile]

Added ios implementation too

I made the class public, do we want it public or open (open is like public, but can be overwritten)
I made the class and the method accessible from Objective-C, do we want that or expect to only be used from Swift?

[imhoffd]

Since we don't have a way to override the functionality class in Android yet, I think we can keep the iOS functionality class just public for now, not open.

Allowing access from Objective-C is fine, unless there's a side effect I'm not aware of.

@ikeith thoughts on the above?

[ikeith]

A few thoughts:

public seems like the right choice to me. I'm having a hard time thinking of a good reason to subclass the plugin, and doing so is likely to cause issues with name collisions. Ultimately, plugin authors can do whatever they want but I don't think we should encourage that by making open the default.

I'm not a big fan of the use of 'Functionality' in this context because that sounds ambiguous to me. It sounds like it might be just helper functions. Ideally, the relationship would be PluginWrapper and Plugin but since the 'wrapper' class is the one exposed to the bridge we can't do that. Maybe 'Implementation'? 🤷‍♂️

Also, would it be worth it to make the template more complicated? I imagine that the most likely point that this separation will break down is when needing to handle an async operation. So providing an example of a completion handler could reduce the risk that authors will just pass the plugin call to the second class and defeat the purpose of the wrapper.

[imhoffd]

The issue is we refer to it as the "plugin class" everywhere, even though it's just an adapter or wrapper. I was hard pressed to come up with a better suffix than Functionality myself. @BorntraegerMarc any thoughts?

Yes, my idea for a more complicated plugin template was to have a squaring plugin. Give it a number, and it returns the square of that number. If you have ideas for an async example, I'm all ears.

[ikeith]

What about a very simple network operation? Just make an http request to return success or failure?

[imhoffd]

How about a plugin that returns the "current working directory" of an app? It would just return the app's root directory on the device.

[BorntraegerMarc]

IMO the naming should be without a suffix. The Plugin class should have Plugin as a suffix. I think it's straightforward if this convention is followed throughout the project.

e.g. Camera & CameraPlugin

it's definitely understandable if this architecture is documented (with code comments). Maybe we should add a default code comment in the template? e.g:

/**
* The Plugin class is but a thin wrapper around the actual functionality. For more information see the following link: xxx (we could link to a github issue with the conversation about the design decision)
*/

[jcesarmobile]

The problem with using CameraPlugin or CameraWrapper for the actual plugin class is, on Android the JS name is automatically taken from the plugin class that has the NativePlugin annotation, so we can't use a different name than the JS API name.
On iOS it's similar, but since the name is taken from the Plugin.m macros, I think it could be fixed easily.

Dan, I think Ian means that it would be more useful to have an async function instead of a more complex sync function, since calling a sync function is easier. But maybe we can have both. But at the moment I can't think of a simple native API we could use.

Also, we need to have in mind that the docs have a plugin guide based on the old code and we will need to rewrite it entirely as we are changing the whole template.

[imhoffd]

We could update the annotation to accept a name for the JS plugin. I really like @BorntraegerMarc's suggestion.

@jcesarmobile We'll need to start a 3.x branch in the Capacitor site shortly. I'm hoping to do that as soon as things settle down with the rewrite/guides being added/etc.

[ikeith]

If we can update the Android annotation to support a different JS name over the class name, then I second the Foo/FooPlugin naming. The iOS code can already support multiple names: the swift class name and the Obj-C class name and the JS name, none of which have to be identical.

As for the sample code, having a simple sync call along with an async call would be but great but I can't think of any native API other than network requests which are inherently asynchronous and make sense cross-platform. If we can't come up with an idea to include in the code, we could just provide an example of a network call in the documentation (assuming anyone reads it).

[imhoffd]

NativePlugin already supports this: https://github.com/ionic-team/capacitor/blob/main/android/capacitor/src/main/java/com/getcapacitor/NativePlugin.java#L28-L32

[imhoffd]

Okay, I pushed the proposed changes. @jcesarmobile @ikeith @BorntraegerMarc

[jcesarmobile]

why is this needed?
Both are dependencies in the Podfile, so should be able to find them without this

[imhoffd]

I think @ikeith was getting it to build without cocoapods?

[jcesarmobile]

Shouldn't iOS tests test the implementation class instead of the plugin class?