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

Remove dependency on Flutter; upgrade dependencies #90

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

michaelstepner
Copy link

@michaelstepner michaelstepner commented Jul 29, 2024

In the jmap-dart-client library, Flutter is only used to execute tests. This PR removes the dependency on flutter and replaces testing using flutter test with dart test, using the dart test package: https://pub.dev/packages/test

The modified code files have also been linted by the default Dart linter, which has generated a large number of whitespace changes in the diff. If you'd like me to revert the whitespace changes and limit the diff to meaningful changes, just let me know. (Done!)

Motivation: I'm trying to use this library in a Dart codebase that does not use Flutter, and Flutter is a heavy dependency which was causing package versioning conflicts. Upon investigation, I realized the jmap-dart-client package isn't using any flutter-specific features, which makes sense, since it is a backend library and flutter is an SDK for frontend UI.

In this JMAP library, Flutter is only used to execute tests. I've replaced `flutter test` with `dart test` using the dart test package: https://pub.dev/packages/test

The modified code files have also been linted by the default Dart linter.
@hoangdat
Copy link
Member

hoangdat commented Aug 8, 2024

Hi @michaelstepner, thanks for your contribution.
Your change also includes a lot of format change (dart linter). Can we eliminate it?

Thanks and BRs

This reverts the formatting changes from commit a5b8e19.
@michaelstepner
Copy link
Author

@hoangdat Thanks for reviewing, I've eliminated the formatting changes in the latest commit to this PR.

pubspec.yaml Outdated Show resolved Hide resolved
Was previously installed automatically alongside Flutter
Was previously installed automatically alongside Flutter
@michaelstepner
Copy link
Author

@dab246 the CI failures should be resolved in commit a0a16df

@dab246
Copy link
Member

dab246 commented Aug 9, 2024

@dab246 the CI failures should be resolved in commit a0a16df

Still failed. Please check log

Screenshot 2024-08-09 at 11 55 25

@michaelstepner
Copy link
Author

@dab246 would you prefer to upgrade to v3 of the Dart SDK, or downgrade to an older version of the test package? I installed v2.19 in the ci.yml Github Action because the existing copy of pubspec.yaml requests an outdated version of the Dart SDK:

environment:
  sdk: ">=2.19.2 <3.0.0"

In a separate branch of my fork (update-dependency-versions), I upgraded the Dart SDK and all packages to their latest versions, and verified that the tests run successfully.

When upgrading to dio 5.0.1 or higher, had to replace:

DioAdapter(dio: dio);

with

DioAdapter(dio: dio, matcher: const UrlRequestMatcher());
@michaelstepner michaelstepner changed the title Remove dependency on Flutter Remove dependency on Flutter; upgrade dependencies Aug 9, 2024
@michaelstepner
Copy link
Author

michaelstepner commented Aug 9, 2024

would you prefer to upgrade to v3 of the Dart SDK, or downgrade to an older version of the test package?

Commit 2e0c3e8 upgrades the Dart SDK to v3 and upgrades the package dependencies to their latest versions. Commit ec04a1c does so without the linter changes. I've verified that all the tests pass locally:

$ dart test
Building package executable... (1.8s)
Built test:test.
00:02 +84: All tests passed!

@dab246 you can test again in the Github Actions CI system, or let me know if you'd prefer a different approach.

michaelstepner and others added 2 commits August 9, 2024 10:38
Adds back the substantive changes to DioAdapter() from 2e0c3e8, without the linting changes removed in 9f3975c
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.

3 participants