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

export OneSignalPlugin so it can be imported in consumer typescript code #806

Merged
merged 1 commit into from
Aug 16, 2022

Conversation

wilgert
Copy link
Contributor

@wilgert wilgert commented Aug 9, 2022

Description

This exports the OneSignalPlugin class so it can be used in TypeScript.

Details

Motivation

In the 3.0.x versions of this plugin we could do import { OneSignalPlugin } from 'onesignal-cordova-plugin'; to get strong typing while dependency injecting the oneSignal plugin in our Angular code. With version 3.1.0 this stopped working. To fix it a export keyword was added.

Manual testing

Tested the app was building again after adding this keyword.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@nan-li nan-li self-assigned this Aug 9, 2022
@nan-li nan-li self-requested a review August 9, 2022 15:19
Copy link
Contributor

@nan-li nan-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this and submitting this PR.

@nan-li nan-li merged commit 7dcbe64 into OneSignal:main Aug 16, 2022
@nan-li nan-li mentioned this pull request Aug 16, 2022
@nunoarruda
Copy link
Contributor

nunoarruda commented Aug 22, 2022

Just for your curiosity, this looks like a matter of default export vs named export.

I've been using this plugin on a TypeScript project for a long time and since it was using a default export I had to import it like import OneSignal from 'onesignal-cordova-plugin' and I had strong typing and everything.

In sum...

Using default export:
Exporting: export default OneSignal
Importing: import OneSignal from 'onesignal-cordova-plugin' (you can rename OneSignal to whatever you like)

Using a named export:
Exporting: export class OneSignalPlugin
Importing: import { OneSignalPlugin } from 'onesignal-cordova-plugin' (you have to import it exactly as OneSignalPlugin and can only renamed it using an alias like import { OneSignalPlugin as OS } for example)

@ssisniega
Copy link

Hello boys!
I am using version 3.3.0 of the plugin.
I am getting this error: I cannot reference an instance method (in my example this.notificacionRecibida(...) ) in the delegate that receives the open push information.
How do you suggest I can fix it? If I inject the plugin dependency I get an error.

image

Thanks!

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