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

fix: replace flutter_keyboard_visibility with flutter_keyboard_visibility_temp_fork to support Flutter/Wasm target #2293

Merged
merged 6 commits into from
Sep 27, 2024

Conversation

EchoEllet
Copy link
Collaborator

Description

Replace flutter_keyboard_visibility with flutter_keyboard_visibility_temp_fork to support Flutter/Wasm target.

This is not a future-proof solution and we should replace the plugin, we might have our own solution in quill_native_bridge or develop a separate plugin.

Related Issues

Type of Change

  • New feature: Adds new functionality without breaking existing features.
  • 🛠️ Bug fix: Resolves an issue without altering current behavior.
  • 🧹 Code refactor: Code restructuring that does not affect behavior.
  • Breaking change: Alters existing functionality and requires updates.
  • 🧪 Tests: Adds new tests or modifies existing tests.
  • 📝 Documentation: Updates or additions to documentation.
  • 🗑️ Chore: Routine tasks, or maintenance.
  • Build configuration change: Changes to build or deploy processes.

…lity_temp_fork to support wasm and be able to target latest version of compileSdkVersion
@EchoEllet
Copy link
Collaborator Author

EchoEllet commented Sep 27, 2024

Was able to build the app for Flutter/WASM, however still having an issue running the example app:

Details

image

Which seems to be related to hydrated_bloc. See bloc #4230

After removing hydrated_bloc:

image

Seems like a simple issue that require minor changes.

Update:

image

Was able to fix those minor issues and seems to be running without any noticeable issues with Flutter/Wasm target on the web.

@EchoEllet
Copy link
Collaborator Author

EchoEllet commented Sep 27, 2024

It looks like kIsWeb is true on Flutter/Wasm, and this conditional import uses web/quill_controller_web_stub.dart:

import 'web/quill_controller_web_stub.dart'
    if (dart.library.html) 'web/quill_controller_web_real.dart';

So our code expects to use quill_controller_web_real.dart due to the kIsWeb check when the conditional import is invalid for Flutter/WASM which uses quill_controller_web_stub.dart that throws exception.

The alternative of dart.library.html is dart.library.js_interop. Since we don't need quill_controller_web_real.dart unless #2220 (see 1998) is fixed, commented the code and removed it completely from being used, left a TODO with reference so other developers know why this is still not removed, I will address as soon as I can. We might also consider moving this functionality to quill_native_bridge_web.

This comment will be updated soon.

Copy link
Collaborator

@AtlasAutocode AtlasAutocode left a comment

Choose a reason for hiding this comment

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

It bothers me to see a package with a label 'temp_fork'.

As a user, when I see this, I immediately worry that the main package is not stable, is going to have big breaking changes, or the function is a kludge and is likely to disappear. I know this is not your intent, but it sets up subliminal red flags.

Is it possible to remove 'temp'?

I have no problem with the code.

@EchoEllet
Copy link
Collaborator Author

EchoEllet commented Sep 27, 2024

Is it possible to remove 'temp'?

flutter_keyboard_visibility_temp_fork is the name of the package and doesn't reflect the package itself, I could have named it flutter_keyboard_visibility_experimental though it doesn't necessarily mean it's experimental, the name is not a reflection of functionality. Chose to name it flutter_keyboard_visibility_temp_fork to make it clear enough to users that the support of this package will be dropped once we find a replacement in flutter_quill. If there is a replacement or update, we will deprecate the package and update flutter_quill.

Already updated the README.md of this fork with the details so you might want to see it. It doesn't have many changes except fixing those 2 critical issues.

I immediately worry that the main package is not stable, is going to have big breaking changes

This is why I suggested that we should be more cautious in the future when adding packages, especially plugins, the original package hasn't been updated in pub.dev for 2 years and on GitHub for 9 months, there were minor changes since those last 2 years, you will notice that they still didn't migrated to flutter_lints from pedantic which was last published before 3 years. See their commit history.

Why I'm not worried about this?

  • I do have access to flutter_keyboard_visibility_temp_fork and should be able to publish any necessary changes
  • Since this functionality is usually only expected for mobile, it should be easier to implement since I'm already familiar with both Kotlin and Java (from Android, XML, and Java world), (not really with Swift though it's already straightforward). For the web, most of us are all familiar with Javascript/Typescript (though I haven't really used a production project using JS for the last few years). Should be easy enough to use web
  • It's most likely that flutter_keyboard_visibility won't get updated anytime soon (even if it will, we should not expect it). Addressing those issues is critical since users are often worried when a new technology in the framework doesn't work with a dependency or package like Flutter Quill even if they don't use it, they will shift to other alternatives.

I'm still in the middle of #2230 so I can't develop a replacement for flutter_keyboard_visibility, we haven't even discussed if this functionality should be in quill_native_bridge or in a separate package quill_keyboard_visibility.

but it sets up subliminal red flags.

I agree though I don't see any risk from this PR, we only use this plugin from one place in raw_editor_state.dart, and even if we decide to use another package or major breaking changes are introduced, updating the related code is quite minimal.

@EchoEllet EchoEllet marked this pull request as ready for review September 27, 2024 18:42
@EchoEllet EchoEllet merged commit 18bb358 into master Sep 27, 2024
2 checks passed
@EchoEllet EchoEllet deleted the fix/wasm-build-failure branch September 27, 2024 18:44
@fdwl
Copy link

fdwl commented Sep 30, 2024

The project failed to compile due to a conflict in dependencies. Here's the translated and elaborated error message:

What went wrong:
Execution failed for task ':app:mergeLibDexBerrytraceDebug'.
> A failure occurred while executing com.android.build.gradle.internal.tasks.DexMergingTaskDelegate
   > There was a failure while executing work items
      > A failure occurred while executing com.android.build.gradle.internal.tasks.DexMergingWorkAction
         > com.android.builder.dexing.DexArchiveMergerException: Error while merging dex archives: 
           Type com.jrai.flutter_keyboard_visibility.BuildConfig is defined multiple times: 
           /Volumes/noah/work/babify_app/build/flutter_keyboard_visibility/.transforms/80052641c9a712d2d3284697f65c1e3f/transformed/classes/classes.dex, 
           /Volumes/noah/work/babify_app/build/flutter_keyboard_visibility_temp_fork/.transforms/f307c2e859924a658e4ca8c7d61427e1/transformed/classes/classes.dex
           Learn how to resolve the issue at https://developer.android.com/studio/build/dependencies#duplicate_classes.

This error indicates that there are duplicate class definitions in your project. Specifically, the BuildConfig class from the flutter_keyboard_visibility package is defined multiple times. This is likely because you have two versions of the same package in your project:

  1. The original flutter_keyboard_visibility package

  2. A temporary fork of the same package, possibly named flutter_keyboard_visibility_temp_fork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants