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

chore: deprecate spell checker service #2255

Merged
merged 3 commits into from
Sep 21, 2024

Conversation

EchoEllet
Copy link
Collaborator

@EchoEllet EchoEllet commented Sep 20, 2024

Description

Deprecate SpellCheckerService as a breaking change will arrive soon.

Make it clear to the users that the SpellCheckerService should not be used as breaking change will be arriving soon, we provided an implementation in the example only (with the usage of simple_spell_checker) and mentioned that this is not a feature implemented in the project, which doesn't mean we can remove the SpellCheckerService right away. We should at least give the users a few weeks (which is not enough) if we're going to introduce this change in a minor breaking change release (10.7.0).

Should also be mentioned that, will cause issues for most projects that use it since pub.dev (pub_semver) expects the version to follow the same format and guidelines as semver. Some projects use tools to automate the process of updating the dependencies to the latest non-breaking release and then publishing a new version (sometimes).

flutter pub upgrade --major-versions will update them even if breaking changes are introduced. flutter pub upgrade will update to the latest non-breaking release. It assumes that 10.7.0 is not a breaking change.

Temporarily removed the docs as it can be used to indicate that this is a stable feature. We do want to limit the damage that can be caused by fixing #2246.

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.

Suggestions

I highly suggest marking new features (including #2230) as experimental, marking them as experimental doesn't necessarily mean the feature is likely to be broken or incomplete but more like this is not stable and can be changed in a way that users will also have to update their code in order to use newer versions. This gives us the opportunity to fix issues without having to worry about releasing a new major breaking release.

We should be more direct and straight with the users to explain the breaking changes if having minor breaking changes such as updating the name of something (or correct misspellings, example without breaking change for internal use only class) in minor versions is acceptable (while mentioning this in README)?

In all cases we should be more cautious about adding new APIs and should be at least marked as experimental, I prefer that we don't add any new features for the next year (or even more) once we fix all issues and code quality issues. This will be a long way and we don't want to give promises we can't keep up to. So no guarantee that this will be worth it.

But having more incomplete features (which is very often) with more issues, and even more code quality issues doesn't seem to be a solution at all, if you disagree, you might see example projects are moving away from the project. Fixing this is simple, which doesn't mean it's simple to execute.

We had more discussion about this in #2235.

Copy link
Collaborator

@CatHood0 CatHood0 left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to deprecate this feature. For now these changes are more than enough because SpellCheckerService is in a very early stage. And as you mentioned, it's best to release these new features only when they are stable (which in this case is the least likely, since I couldn't provide a more compact solution that would fit the needs of the developers).

For now I'll take the time to start fixing problems instead of adding new features, since so many new things really add more complexity to the project.

@EchoEllet
Copy link
Collaborator Author

Thanks. Users should still be able to use simple_spell_checker even after the breaking change.

@CatHood0
Copy link
Collaborator

Users should still be able to use simple_spell_checker even after the breaking change.

I think that's fine, but I'm planning to break changes anyway because i need to improve the current implementation of it, which is not only slow, but cumbersome and sometimes doesn't even fulfill its basic function. It will take some time anyway. Thanks for taking the time and effort.

@EchoEllet
Copy link
Collaborator Author

I think that's fine, but I'm planning to break changes anyway because i need to improve the current implementation of it, which is not only slow, but cumbersome and sometimes doesn't even fulfill its basic function. It will take some time anyway. Thanks for taking the time and effort.

If you're going to introduce breaking changes and in case you have the time, you probably want to separate support of each language into its own package, use melos to automate the publishing since it's can be time-consuming.

There are other things to consider such as web support, how will these files will be supported? especially for the web, don't know how Flutter Web loads files but shouldn't be more memory performance efficient when loading them if they were file assets instead? Do they slow down the app loading on the web if deployed?

Of course, it might not be a priority until now or you might not have the time at the moment for those changes.
But those kinds of issues you probably want to address all at once when introducing a major breaking change in a major version, instead of multiple major versions like what we have here.

@EchoEllet EchoEllet marked this pull request as ready for review September 20, 2024 23:50
@CatHood0
Copy link
Collaborator

If you're going to introduce breaking changes and in case you have the time, you probably want to separate support of each language into its own package, use melos to automate the publishing since it's can be time-consuming.

Thanks a lot for the suggestion, I'll probably use it. And I guess since you mentioned making a package for each language, it gave me an idea of ​​how I'll do this.

There are other things to consider such as web support, how will these files will be supported? especially for the web, don't know how Flutter Web loads files but shouldn't be more memory performance efficient when loading them if they were file assets instead? Do they slow down the app loading on the web if deployed?

Apparently the website is able to load the dictionary files, but since they are large (normally, 80 MB is too much for such a small feature) it takes longer for a page to load.

Of course, it might not be a priority until now or you might not have the time at the moment for those changes.
But those kinds of issues you probably want to address all at once when introducing a major breaking change in a major version, instead of multiple major versions like what we have here.

Yes, for now I will keep the package the same while I create an implementation that is more in line with what is required and that is used by developers.

I say this because I have noticed that some people make forks to disable all other languages ​​and leave only the one they will use. This gives me the impression that, as you mention, having languages ​​in separate packages would allow all developers to have only the languages ​​they are interested in instead of having everything (which they will probably only use one or two of those dictionaries)

Thank you very much for your recommendations. I really appreciate the help.

@singerdmx
Copy link
Owner

Can we please merge some of PRs before making more?

@EchoEllet EchoEllet merged commit af6060f into master Sep 21, 2024
2 checks passed
@EchoEllet EchoEllet deleted the chore/deprecate-spell-checker-service branch September 21, 2024 02:10
CatHood0 pushed a commit to CatHood0/flutter-quill that referenced this pull request Sep 21, 2024
* chore: deprecate SpellCheckerService

* docs(readme): update docs to reflect the change

* chore: deprecate DefaultSpellCheckerService and SpellCheckerServiceProvider, ignore all warnings
CatHood0 pushed a commit to CatHood0/flutter-quill that referenced this pull request Oct 28, 2024
* chore: deprecate SpellCheckerService

* docs(readme): update docs to reflect the change

* chore: deprecate DefaultSpellCheckerService and SpellCheckerServiceProvider, ignore all warnings
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