-
-
Notifications
You must be signed in to change notification settings - Fork 405
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
Update js dependency and migrate to web #680
Update js dependency and migrate to web #680
Conversation
This PR now also migrates from |
Is there any date to merge this and create a new version?? |
@JgomesAT I don't even know if this will ever get merged as the repo owner is not very active currently. |
With the version of your repo I have an error in compilation: ../../../../.pub-cache/git/flutter_secure_storage-6ac827e457393b5c654c1abda40d31e2cb93f62c/flutter_secure_storage/lib/flutter_secure_storage.dart:268:17: Error: The getter 'onCupertinoProtectedDataAvailabilityChanged' isn't defined for the class 'FlutterSecureStoragePlatform'.
|
You probably need to override the other packages as well |
What other package?? |
As described above, the platform interface and web submodules (and probably also linux etc. |
but this library is part of the flutter_storage I don't have this library in the pubspec |
IF I put your repo in the pubspec dependencies: and the old one in the dependencies_override dependencies: dependency_overrides: I have a successful build |
@JgomesAT Then, you are using the official version
You have all these packages in your |
I think that is an issue of your code I only use your repo, and I removed all references of package flutter_secure_storage: ^9.0.0 |
This is an issue with your current setup. |
I'm sorry, but I didn't understand, this behavior is strange I need to add all submodules, in a possible final merge of the library this won't be necessary, will it? example I need to add this in the override: flutter_secure_storage_platform_interface: |
Exactly, after merging and releasing these changes properly to pub.dev, this won't be necessary anymore. |
@@ -13,7 +13,8 @@ dependencies: | |||
flutter_secure_storage_platform_interface: ^1.0.1 | |||
flutter_web_plugins: | |||
sdk: flutter | |||
js: ^0.6.3 | |||
js: '>=0.6.3 <1.0.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
js: '>=0.6.3 <1.0.0' |
The migration to the new js-interop approach also means getting rid of the package:js.
https://dart.dev/interop/js-interop/package-web
See also these comments firebase/flutterfire#12027 (comment) from Kevin Moore (Flutter Web PM)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, thanks for reminding me about this... Currently, I don't have enough free time to also address this, but hopefully I will do so soon!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries 😌 I hear your struggle to find time to maintain/contribute to open-source projects!
Will this be merged soon? |
Can this get merged? @ThexXTURBOXx |
I think yes. Someone might want to migrate some stuff to |
When will you make the merge of this? |
@juliansteenbakker Since I have seen that you are about to incorporate this, I want to mention that the js_interop migration from #698 should also be considered. A mixture of this PR right here and #698 is probably the best way to go. |
Hi @ThexXTURBOXx , that would be awesome ! I am planning on releasing a smaller version with bug fixes first today, and a new major version with some breaking changes and possibly this PR somewhere in the end of this week. |
@juliansteenbakker Done! :) |
@@ -1,7 +1,7 @@ | |||
name: flutter_secure_storage_web | |||
description: Web implementation of flutter_secure_storage. Use flutter_secure_storage for the full flutter package. | |||
repository: https://github.com/mogol/flutter_secure_storage | |||
version: 1.1.2 | |||
version: 1.2.0 | |||
|
|||
environment: | |||
sdk: ">=2.12.0 <4.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good, however the only thing that's a bit annoying is that the web package is available from dart 3.3, which is basically the latest flutter version, meaning that if we publish this, everyone is required to run the latest Flutter version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is true. I have pushed the constraints for web
down as far as possible. Older versions lack features that are (IMHO) critical for the functionality of flutter_secure_storage
. Maybe, the constraints could be pushed even lower than that using some hacky workarounds, but I am unsure.
This is also one of the reasons why it makes sense to apply this PR (and #698 for that matter) only after a major version bump
I have released v9.1.0 which has all fixes that are possible for the current lower sdk version. So i will now merge this, and the next release (v10) will use the higher sdk version. |
7e2321a
into
juliansteenbakker:develop
This allows newer versions of
js
being used in conjunction withflutter_secure_storage
. Also, ashtml
is about to get phased out, this PR already migrates toweb
as it is recommended practice to do so.Fixes #670
Fixes #679
Fixes #683
If you want to use this now, you can do so via a
dependency_override
:You probably need to also override the other subpackages similarly (I won't explain how to do that. Just do it as I have shown with the main package above).