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

Reject promise returned from init if script loading fails #60

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ljuborados
Copy link

Firstly, semantically doesn't make much sense to resolve a promise which obviously failed.

Apart from that, we're handling the logic for when the user has AdBlock turned on, where we show a popup asking the user to disable the AdBlock in able to receive push notifications (which I suspect most developers would do), rejecting the promise returned from init removes the need for workarounds while still keeping the functionality untouched for other cases.

@rgomezp
Copy link
Contributor

rgomezp commented May 25, 2022

This would be a breaking change so we would need to include this in a potential major release.

Furthermore, we don't want to raise an error for simply having ad block turned on.

rejecting the promise returned from init removes the need for workarounds while still keeping the functionality untouched for other cases.

can you please elaborate a bit more on this point? what workaround are you referencing here? and what other cases?

@ljuborados
Copy link
Author

Would accepting a onError callback be a better option?
Shouldn't be a breaking change then as the functionality would remain the same as now.

Well, in the current state, library doesn't let you handle the case when AdBlock is on and blocking One Signal from loading at all. We've used some workarounds like pinging OneSignal URL and checking if the request goes through but this is heaps cleaner.

@ljuborados
Copy link
Author

I'd also expect basically any production implementation to want to handle blocked notifications, so IMHO is an important feature.

@rgomezp rgomezp added the Enhancement / Feature New feature or request label Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement / Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants