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

dart_ping_ios should become dart_ping_flutter #28

Open
guyluz11 opened this issue Jul 23, 2022 · 5 comments
Open

dart_ping_ios should become dart_ping_flutter #28

guyluz11 opened this issue Jul 23, 2022 · 5 comments
Labels
blocked Awaiting changes to a dependency

Comments

@guyluz11
Copy link

If I understand correctly dart_ping_ios is the same package as dart_ping but includes flutter parts so that it can interact with flutter system calls like the package for ios.

But what if for example we want to add support for new features that need special android implementation as well as spacial ios implementation as well as Linux Windows Web...

Building a new package for each one will be a lot of projects to maintain.

We can do one project for flutter dart_ping_flutter and call the correct implementation by platform.

My suggestion is to change the project dart_ping_ios to dart_ping_flutter because it is already based on flutter.

@point-source
Copy link
Owner

It's been a while since I looked at that but I seem to recall it being such that you cannot use the dart_ping_ios package on it's own. I think it's just a plugin to dart_ping so it wouldn't be totally correct to rename it as you suggest.

That doesn't mean it couldn't be refactored / rebuilt to behave the way you mean. It would just be dart_flutter encapsulating dart_ping instead of the other way around. Perhaps this would make more sense but the reason I built it the way I did was so that one could use conditional imports to only import the ios / flutter side as necessary.

@guyluz11
Copy link
Author

guyluz11 commented Oct 1, 2022

I built it the way I did was so that one could use conditional imports to only import the ios / flutter side as necessary.

That could be handled inside the package and for the developers it will be easier and more convenient.

seem to recall it being such that you cannot use the dart_ping_ios package on it's own.

Why not just import dart_ping inside dart_ping_flutter / dart_ping_ios?.

But no need to do a big change if ios is the only one that needs to be supported.

@git-elliot
Copy link
Contributor

git-elliot commented Sep 4, 2023

but the reason I built it the way I did was so that one could use conditional imports to only import the ios / flutter side as necessary.

Can you tell me how to conditionally import dart_ping_ios?
What I want is something like this (Hypothetically)

import 'package:dart_ping_ios/dart_ping_ios.dart'
    if (dart.library.io) 'package:network_tools_flutter/src/shim/dart_ping_ios.dart'
    if (dart.library.swift) 'package:dart_ping_ios/dart_ping_ios.dart';

I don't want to import your dart_ping_ios package on other platforms because it makes my package compatibility limited to android/ios which conversely supports desktop just like dart_ping.

What I think is dart.library.swift doesn't exists.. I saw on internet only io, html, and js are there but say if what you are saying about conditional imports is true then why didn't you do it in your dart_ping project for flutter_icmp_ping.

P.S.: I think dart.library.io represents all native platforms i.e., iOS, Android, Linux, Macos, and Windows and dart.library.html represents web

Source : https://stackoverflow.com/questions/66840203/flutter-conditionnal-import-mobile-vs-desktop

I guess I have to be satisfied with only Platform.isIOS wrapped everywhere I use your dart_ping_ios package

@point-source
Copy link
Owner

point-source commented Sep 6, 2023

Ah yep, you're right. I didn't realize these conditionals were restricted only to dart.library.x directives. This would, as you've pointed out, interfere with the ability to conditionally import based on platform (ios/android vs desktop) rather than dart target (native vs web).

In the greater scheme, I currently cannot come up with any mechanism which allows me to support all flutter platforms and native (non-flutter) dart at the same time. I have to trade between non-flutter and iOS since the iOS implementation relies on flutter_icmp_ping. Once this issue gets resolved, this should become much easier to deal with since I can then bundle the swift/Obj-C code for iOS into a native (non-flutter) dart binary.

Perhaps you can try to override the detected platforms on your package as described here: https://dart.dev/tools/pub/pubspec#platforms

Is the issue that your package isn't being detected as having support even when it does or does it actually fail to work on desktop when you import the dart_ping_ios package? Note that DartPingIOS.register(); only needs to be called once and it only needs to be called before the first time you use dart_ping on an iOS platform. So simply using Platform.isIOS one time at application run to determine whether to call DartPingIOS.register(); should suffice to enable iOS support and still work on every platform other than web.

@point-source point-source added the blocked Awaiting changes to a dependency label Sep 6, 2023
@git-elliot
Copy link
Contributor

git-elliot commented Sep 7, 2023

@point-source my package doesn't fail on other platforms because of dart_ping_ios, it just doesn't show platforms on pub.dev correctly. Thanks for the solution - I'll add in platforms section of pubspec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Awaiting changes to a dependency
Projects
None yet
Development

No branches or pull requests

3 participants