Swift Package Manager support?

[powerje]

Is there a roadmap or discussion around SPM support for this library? This is something I'd be happy to contribute to if the team is open to it.

[mpodwysocki]

@powerje Yes, that is on our roadmap for our SDK and will keep you posted on our progress!

[mpodwysocki]

@powerje can you please review this work here and comment? #46

[powerje]

Thank you for looking into this so quickly!

When I try to pull this in via the feat/spm branch in Xcode I get an error due to the Package.swift file not being in the root of the project. I'm not sure if it is possible to direct Xcode to a subfolder to find Package.swift?

I cloned the repo and used the method of dragging the src folder (the folder containing the Package.swift file) into my project. Once I added the dependency to a target that target no longer builds, interestingly Xcode shows no error output, it just fails 😅.

Building my target from the command line does produce a potentially useful error:

▸ Compiling SBTemplateRegistration.m
    Could not read serialized diagnostics file: Cannot Load File: Failed to open diagnostics file (in target 'WindowsAzureMessaging' from project 'WindowsAzureMessaging')
▸ Compiling SBStoredRegistrationEntry.m
    Could not read serialized diagnostics file: Cannot Load File: Failed to open diagnostics file (in target 'WindowsAzureMessaging' from project 'WindowsAzureMessaging')
▸ Compiling SBStaticHandlerResponse.m

I think that running swift build in the src directory should compile cleanly but I do see some errors, I think due to some headers not importing Foundation (which IIRC Xcode implicitly imports, which is why before now that was not a problem):

$ pwd
/Users/jep/Development/azure-notificationhubs-ios/src
$ swift build -Xswiftc "-sdk" -Xswiftc "`xcrun --sdk iphonesimulator --show-sdk-path`" -Xswiftc "-target" -Xswiftc "x86_64-apple-ios13.0-simulator" -v
... lots of output here ...
/Users/jep/Development/azure-notificationhubs-ios/src/WindowsAzureMessaging/WindowsAzureMessaging/Helpers/SBTokenProvider.h:5:27: note: add a super class to fix this problem
@interface SBTokenProvider : NSObject{
                          ^
                           : NSObject
4 warnings and 15 errors generated.
1 error generated.

I added the Foundation import to SBTokenProvider which seemed to fix that issue but there are a few more:

/Users/jep/Development/azure-notificationhubs-ios/src/WindowsAzureMessaging/WindowsAzureMessaging/Helpers/SBRegistrationParser.m:6:9: fatal error: 'SBTemplateRegistration.h' file not found
#import "SBTemplateRegistration.h"
        ^~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/jep/Development/azure-notificationhubs-ios/src/WindowsAzureMessaging/WindowsAzureMessaging/SBNotificationHub.m:13:9: fatal error: 'UIKit/UIKit.h' file not found
#import <UIKit/UIKit.h>
        ^~~~~~~~~~~~~~~
/Users/jep/Development/azure-notificationhubs-ios/src/WindowsAzureMessaging/WindowsAzureMessaging/Helpers/SBNotificationHubHelper.m:6:9: fatal error: 'SBTemplateRegistration.h' file not found
#import "SBTemplateRegistration.h"
        ^~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/jep/Development/azure-notificationhubs-ios/src/WindowsAzureMessaging/WindowsAzureMessaging/Helpers/SBLocalStorage.m:9:9: fatal error: 'SBTemplateRegistration.h' file not found
#import "SBTemplateRegistration.h"

I can look into this more next week but I'm really not sure about the UIKit import issue

[mpodwysocki]

@powerje yes, that has been an issue here as well, and hoping there was an easy answer for that. I did add UIKit explicitly as a framework to see if that helped. Also made the change to the header file

[powerje]

Okay, I have it building now - I'm not sure why the UIKit import was failing, but building with xcodebuild seems to work if I change the imports to point to ../SBTemplateRegistration.h - I think this shouldn't break anything but I'm not sure why it's required. I also modified Package.swift slightly to point to the library specifically.

Because the Package.swift is not at the project root I had to pull the src folder into my project directly to import it. I think we should put the Package.swift file in the root of the project to allow for easily importing into Xcode without having to clone this project.

For reference, this is the demo project & branch I pulled WAM into:
https://github.com/powerje/SearchController/tree/azure_spm_attempt

and this is the PR with my changes to feat/spm:
#47

[brannon]

Hey @powerje, this is unrelated to the SPM discussions, but we are working on an updated SDK with a much simpler interface:
https://github.com/Azure/azure-notificationhubs-ios/releases/tag/3.0.0-preview1

If you are just getting started with integrating Notification Hubs in your app, we'd love if you could take a look at the new SDK and provide any feedback you might have.

Thanks!

[powerje]

@brannon thanks for sharing! We will be working on adding notification support to this project soon and I will be sure to give you feedback when we start. I'd really like to be able to use SPM to use this SDK to avoid manually importing or introducing another package manager to the project.

With that in mind I have updated #47 and rebased against sdk-v3.

Does that look like an acceptable solution to getting SPM working with this project?

[powerje]

@brannon I've started incorporating the SDK into my application. The API surface is a lot simpler now!

One thing I noticed that I'd like to see changed is: +initWithConnectionString:hubName, the init at the beginning makes me think that this should be an instance method, and I'd expect it to return the instance it creates.

I think a more idiomatic name would be something like:
setupWithConnectionString:hubName

With a name like that I don't expect it to return a value.

This function looks a little off in Swift code:
MSNotificationHub.initWithConnectionString("foo", hubName: "bar")

I think it would be nice to use the NS_SWIFT_NAME_MACRO to make this a little more natural on the Swift side:

+ (void)initWithConnectionString:(NSString *)connectionString hubName:(NSString *)notificationHubName NS_SWIFT_NAME(setup(connectionString:hubName:));

Which makes the Swift code read more naturally:

MSNotificationHub.setup(connectionString: "foo", hubName: "bar")

As a side note, I've updated #47 against the latest sdk-v3 changes.

[powerje]

A few other things that I'm thinking might be useful:

Add optional methods to MSNotificationHubDelegate for receiving errors when registering for APNS and another for receiving the token. Most folks probably wont need these in production (unless they're using an additional backend to send pushes) but they're probably helpful to folks when debugging / setting up push.

[mpodwysocki]

@powerje If you're willing to help out with the following, we'd gratefully accept it:

Add optional methods to MSNotificationHubDelegate for receiving errors when registering for APNS and another for receiving the token. Most folks probably wont need these in production (unless they're using an additional backend to send pushes) but they're probably helpful to folks when debugging / setting up push.

[mpodwysocki]

@powerje ok, will update the API, as you're right, the whole init doesn't make sense in a static context. Since this is a preview, I will put it out there for a rename with the macro. Keep the feedback coming, thank you!

[mpodwysocki]

@powerje Is there a reason we hard copied our files into the include directory versus just making a symlink?

[powerje]

@mpodwysocki that's a good point, symlinks might be preferred. A reason to do this (which I did not do here) would be to hide some implementation details from consumers in the public headers. For example, MSTaggable isn't necessary as part of the public API of MSInstallationTemplate - but it is a required implementation detail when building the library itself.

We could, I think, modify the header in the include folder to not conform to MSTaggable and delete MSTaggable.h from the include folder. We lose that ability if we symlink the headers.

That said, I didn't take advantage of that here so symlink would probably be preferable.

[mpodwysocki]

@powerje

That said, I didn't take advantage of that here so symlink would probably be preferable.

Once you finish up with the current PR and we revisit versioning, let's try to see if we can get symlinks to work.

Also, if you have other rename suggestions, I'm all ears such as

• MSInstallation becomes MSNotificationHub.Installation
• MSInstallationTemplate becomes MSNotificationHub.InstallationTemplate

[mpodwysocki]

@powerje closing as we now have includes symlinked. If this doesn't work, let's reopen and revisit.