parse-community / Parse-SDK-Flutter

The Dart/Flutter SDK for Parse Platform
https://parseplatform.org
Apache License 2.0
575 stars 190 forks source link

ci: Fix failing CI due to missing platform interface dependencies #837

Closed Nidal-Bakir closed 1 year ago

Nidal-Bakir commented 1 year ago

New Pull Request Checklist

Issue Description

Closes: #835

Approach

TODOs before merging

parse-github-assistant[bot] commented 1 year ago

I will reformat the title to use the proper commit message syntax.

parse-github-assistant[bot] commented 1 year ago

Thanks for opening this pull request!

codecov[bot] commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (f872cd4) 16.38% compared to head (1ce6960) 16.38%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #837 +/- ## ======================================= Coverage 16.38% 16.38% ======================================= Files 47 47 Lines 2899 2899 ======================================= Hits 475 475 Misses 2424 2424 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

mtrezza commented 1 year ago

Is this a bug that affects only our CI or also the published SDK for developers? If the latter, what is the bug that can occur for developers?

mbfakourii commented 1 year ago

Is this a bug that affects only our CI or also the published SDK for developers? If the latter, what is the bug that can occur for developers?

I tested the version of parse_server_sdk_flutter: ^3.1.3, there is no problem on the developer side, probably there is a problem in CI.

mtrezza commented 1 year ago

Could it be that issues only may occur on specific platforms? Looking at the docs of the path_provider_platform_interface:

This interface allows platform-specific implementations of the path_provider plugin, as well as the plugin itself, to ensure they are supporting the same interface.

I'm asking because:

This PR adds it to the dev dependencies, so by that we already determine that it's not a product bug. Are we sure of that or should it be added to the prod dependencies? If there is an issue in the dev environment with platform compatibility, couldn't there be also an issue in a prod environment that uses the same platform?

Nidal-Bakir commented 1 year ago

I think it's normal. maybe it's the Tree Shaking that removes these packages. And when we depend on these packages directly the Tree Shaking process will keep them.

Looking at the types of packages the Parse_SDK is a Dart Package and not a plugin.

If you look at the pubspec.yaml in the path_provider plugin you will see plugin_platform_interface: ^2.0.0 under the _devdependencies and dependencies that's because it needed in tests and in production.

We need these plugins in tests so it's safe to put them under dev_dependencies and the users of the package will not be affected.

Because pub will NOT take into account the dev_dependencies from our pubspec.yaml From the docs

Dev dependencies differ from regular dependencies in that dev dependencies of packages you depend on are ignored

So do we need new releases? No there is no need for it.

Do we need a better way to test using path_provider? Yes we can do that using DI (Dependency Injection) and not call the path_provider functions directly but through an interface in this case its the abstract class PathProvider

// core_store_directory_io.dart

class CoreStoreDirectory {
  final PathProvider _pathProvider;

  CoreStoreDirectory(this._pathProvider);

  Future<String> getDatabaseDirectory() async {
    return (await _pathProvider.getLibraryDirectory()).path;
  }

  Future<String> getTempDirectory() async {
    return (await _pathProvider.getTemporaryDirectory()).path;
  }

//--> path_provider.dart

 abstract class PathProvider {
  Future<Directory> getTemporaryDirectory();
  Future<Directory> getLibraryDirectory();
  Future<Directory> getApplicationDocumentsDirectory();
}

class PathProviderImpl extends PathProvider {
  @override
  Future<Directory> getApplicationDocumentsDirectory() {
    return path_provider_package.getApplicationDocumentsDirectory();
  }

  @override
  Future<Directory> getLibraryDirectory() {
    return path_provider_package.getLibraryDirectory();
  }

  @override
  Future<Directory> getTemporaryDirectory() {
    return path_provider_package.getTemporaryDirectory();
  }
}
parse-github-assistant[bot] commented 1 year ago

I will reformat the title to use the proper commit message syntax.

parse-github-assistant[bot] commented 1 year ago

I will reformat the title to use the proper commit message syntax.