-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Web Support #2466
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
base: master
Are you sure you want to change the base?
Web Support #2466
Conversation
Also added getActiveNotifications
|
@MaikuB I'll once again put in my request to disable the |
|
Love this test feedback! Failed to load ".../flutter_local_notification_test.dart": Instance of 'VmServiceDisappearedException' |
|
Hi @Levi-Lesches, I would like to know how I can install this alpha version inside my project. Can you please write a simple procedure on how to use it? Thanks so much |
|
Look into git dependencies in your pubspec, and set the URL to my repo and ref to my branch |
thanks so much. Is there an example on how to show a basic notification (title and subtitle) like on the other platforms? I cannot find a simple example in your code. |
|
It's been a while since I've touched this PR, but |
|
Hello @Levi-Lesches is there an update or ETA to have basic functionality merged? |
|
I hear your calls! Foreground callbacks are now supported! await plugin.initialize(
initializationSettings,
// same as other platforms, but now it'll work on web too!
onDidReceiveNotificationResponse: myHandler,
);Oh, but this commit also adds a new package to the mix, which is unpublished, so you'll need to clone this whole repository and use path overrides for Launch details are also now supported! // Same as every other platform!
final launchDetails = await plugin.getNotificationAppLaunchDetails();If a notification is tapped when your tab is closed, it will be re-opened with a bunch of query parameters. Calling this function will read them and clear them so the user doesn't see them for too long. I'm going to clean up the PR now. Here's what I did not add:
|
|
@MaikuB I'm ready for a review! There a handful of lints that are coming up from the I couldn't really think of a great way to unit test this since it's so close to the actual JS layer and there's barely any plain Dart, so I just did the usual in making the really detailed examples. Let me know if you have a specific test case in mind. I also didn't touch changelogs because I see you have a lot of PRs open and didn't want to conflict. This release is non-breaking though! I added snippets in the web-only README, and made some light changes to the main README, but I want to save my main documentation effort for #2477 and adding a web guide to that PR |
|
Thanks I'll take a look when I can do so. Regarding changelogs, that is something I can deal as part of resolving merge conflicts. With that in mind, if your wanted to, you can add details under "vNext" to represent next release. This is optional though |
MaikuB
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for your effort @Levi-Lesches. I left a number of comments throughout the PR. Let me know if you got any questions or comments
| name: flutter_local_notifications_example | ||
| description: Demonstrates how to use the flutter_local_notifications plugin. | ||
| publish_to: none | ||
| version: 1.0.0+0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason for this addition? It's only meant be given anything that gets published. This is also why it's standard for example apps for packages and plugins to omit these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some other changes that appear to be missing from this file
periodicallyShow()andperiodicallyShowWithDuration()is missing code to resolve web implementation within thekIsWebcheck and call the associated methodzonedSchedule()should throw anUnsupportedErrorfor web. In hindsight, plugin likely needs a review to make use of this error instead ofUnimplementedErrorbut better to tackle separately outside of PR
| flutter_local_notifications_web: ^1.0.0 | ||
| flutter_local_notifications_platform_interface: ^9.1.0 | ||
| timezone: ^0.10.0 | ||
| universal_platform: ^1.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this dependency? It's not actually being used by the main plugin and was only used by the example app
|
|
||
| @override | ||
| Future<List<ActiveNotification>> getActiveNotifications() async { | ||
| if (_registration == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this only returns the IDs of each notification. Can it return more details than that? I've only done a quick test but looked like it could return the title and body. Not sure if payload could also be returned
| } | ||
| final List<ActiveNotification> result = <ActiveNotification>[]; | ||
| final Set<int> ids = <int>{}; | ||
| final List<Notification> jsNotifs = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a general comment that applies to rest of PR and I understand there's an element of subjectiveness...but are able to avoid shortening variable names and name them in a way that allows code to be read more closer to an English sentence? Ones that stick out
result->activeNotifications: will give more reasoning for this one as others are more self-explanatory.resultis generic and whilst I can see how the intent was that the result of the method is to return a collection, with how the code functions, it's awkward to say a notification was added to a result.jsNotifs->notificationsjsNotification->notification: note this also avoids inconsistency between how how the plural used for the collection wasn'tjsNotificationsnotif->activeNotification
|
|
||
| /// Initializes the plugin. | ||
| Future<bool?> initialize({ | ||
| DidReceiveNotificationResponseCallback? onNotificationReceived, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rename all instances of onNotificationReceived to be onDidReceiveNotificationResponse?
| /// An optional URL to an image to show in the notification. | ||
| final Uri? imageUrl; | ||
|
|
||
| /// The language code of the notification's content, eg `en-US`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// The language code of the notification's content, eg `en-US`. | |
| /// The language code of the notification's content e.g. `en-US`. |
Besides this, do you know what the default behaviour is when this is left null? If so, can you add a paragraph to describe? Mentioning this though as lang is expressed as optional but a notification always has ties to a language. Presumably it defaults to relying on the language code/locale of the environment that is running the app.
| @@ -0,0 +1,62 @@ | |||
| 'use strict'; | |||
|
|
|||
| // Experimental function to save a notification to the IndexedDB database | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it's not used, can this be removed? This to expose on what is necessary. Besides that, I assume if this was left used by others, it creates additional maintenance overhead in tracking what could be a breaking change if removed
| import 'package:path_provider/path_provider.dart'; | ||
| import 'package:timezone/data/latest_all.dart' as tz; | ||
| import 'package:timezone/timezone.dart' as tz; | ||
| import 'package:universal_platform/universal_platform.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to have the example app written in a way that doesn't depend on this package? Reasons behind this thought
- Attempting to minimise dependencies on packages that aren't official ones as much as possible as they can end up not being maintained, run into issues etc. Avoiding this will also provide guidance for developers on how to handle such scenarios without relying on third party packages
- It's easy to miss that
Platformnow points toUniversalPlatformthat developers run into issues. It looks like an easy change but is actually more significant than it looks. Flutter is more commonly used for mobile but the example app has had a notable change around how platform detection is done just to deal with web and convenience of this example app. Given this is an example app, it can cause developers to think using theuniversal_platformis recommended even for mobile development when it's not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As all of this is based on a proposal as opposed to a standard, could this be removed? It being part of a proposal means it's subject to change and breaking so want to avoid having the plugin in this situation that in turn adds more burden around maintenance
| @@ -0,0 +1,29 @@ | |||
| name: flutter_local_notifications_web | |||
| description: Web implementation of the flutter_local_notifications plugin | |||
| version: 1.0.0 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to mention something I'd be changing but also happy for you to do is that whilst this PR has been open for a while, when web support is available, it'll start of as being part of a prerelease beginning with the -dev.1 suffix
Adds web support. The plan is as follows, taken from #481 (comment):
Basic functionality
show()getActiveNotifications()cancel()andcancelAll()periodicallyShowandperiodicallyShowWithDurationwould throwUnsupportedErrorpendingNotificationRequestwould return an empty listAdvanced Functionality
web.NotificationOptionsgetNotificationAppLaunchDetails()from terminated statePR Cleanup
Custom service workers
Of course,
getNotificationAppLaunchDetailsand setting any handlers would require the ability to customize the service worker. That appears to be blocked on flutter/flutter#145828, we can also register a new service worker at runtime:Where our custom service worker can start off as simple as
It evolved more throughout the PR, but that's the gist