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

[flutter_local_notifications_linux] Fix build failing on Web #1249

Merged
merged 7 commits into from
Aug 3, 2021

Conversation

proninyaroslav
Copy link
Contributor

@proninyaroslav proninyaroslav commented Jul 28, 2021

Web doesn't support FFI, so we need to strip all transitive dependencies at compile time, that using FFI
Fixed #1247

Web doesn't support FFI, so we need to strip all transitive dependencies at compile time, that using FFI
@proninyaroslav proninyaroslav changed the title [flutter_local_notifications] Fix build failing on Web [flutter_local_notifications_linux] Fix build failing on Web Jul 28, 2021
@proninyaroslav
Copy link
Contributor Author

@MaikuB
I made a compile-time stub for the Linux plugin, I also removed the runtime checking for the integer width, because this duplicates the functionality of the dbus package. Now building for the Web happens without any problems.

@MaikuB
Copy link
Owner

MaikuB commented Jul 28, 2021

Thank. To carry on the convo from the issue thread, I probably wasn't clear enough that I also did a conditional import. The difference though is that the stub had empty implementations for all methods. I won't be able to test this PR until later but I would thought that would be needed as difference between this plugin and the connectivity one is that this plugin has platform-specific methods and the overrides take extra platform-specific parameters. Therefore, I would've thought an app is compiling for the web but makes calls directly to the Linux plugin (e.g. https://pub.dev/documentation/flutter_local_notifications_linux/latest/flutter_local_notifications_linux/LinuxFlutterLocalNotificationsPlugin/initialize.html or https://pub.dev/documentation/flutter_local_notifications_linux/latest/flutter_local_notifications_linux/LinuxFlutterLocalNotificationsPlugin/getSystemIdMap.html), it would fail to compile as these won't exist in the stub.

Another thing is I think it'd still be good to re-implement the posix calls as the posix package depends on ffi versions >= 0.1.3 but less < 1.0.0 to avoid potential problems around dependency conflicts with what's baked into the Flutter SDK. There are very few calls to implement that I'd say it's worth doing to remove posix as a dependency

@proninyaroslav
Copy link
Contributor Author

proninyaroslav commented Jul 29, 2021

it would fail to compile as these won't exist in the stub.

Yes, you are right, thanks for the comment. I forgot that the main plugin implementation calls these methods in code. I moved these methods into a separate interface that extends FlutterLocalNotificationsPlatform, and the real implementation and the stub implement it.
In my local repo, I did a revert that disables Linux support and checked the build of the example app for Linux and Web. I have not noticed any problems.

@proninyaroslav
Copy link
Contributor Author

Another thing is I think it'd still be good to re-implement the posix calls

Yes, I'll push it today.

@proninyaroslav
Copy link
Contributor Author

@MaikuB
POSIX calls: d4ec745
Method stubs: e69b18d

Copy link
Owner

@MaikuB MaikuB left a comment

Choose a reason for hiding this comment

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

Left some comments on the PR. Let me know what you think

@MaikuB MaikuB mentioned this pull request Aug 1, 2021
2 tasks
@MaikuB MaikuB merged commit 9a2245c into MaikuB:master Aug 3, 2021
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.

flutter_local_notifications 6.1.0 got error when running in flutter web.
2 participants