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.

Sign up for GitHub

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

Jump to bottom

feat(package_info_plus)!: Migrate to js_interop for conditional export #2622

Closed
wants to merge 1 commit into from
Closed

feat(package_info_plus)!: Migrate to js_interop for conditional export #2622

wants to merge 1 commit into from

Conversation

amrgetment
Copy link

@amrgetment amrgetment commented Feb 28, 2024 •
edited
Loading

Description

I removed html for conditional import and used js_interop to support WASM

Related Issues

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I titled the PR using Conventional Commits.
  • I did not modify the CHANGELOG.md nor the plugin version in pubspec.yaml files.
  • All existing and new tests are passing.
  • The analyzer (flutter analyze) does not report any problems on my PR.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate that with a ! in the title as explained in Conventional Commits).
  • No, this is not a breaking change.

@amrgetment amrgetment changed the title migrate package_info_plus to package:web feat(package_info_plus)!: Migrate to js_interop for conditional export Feb 28, 2024
@amorenew amorenew mentioned this pull request Feb 28, 2024
Closed
@miquelbeltran
Copy link
Member

miquelbeltran commented Feb 28, 2024 •
edited
Loading

Why is the description saying "Fix for a device_info_plus ticket" but the code is package info plus?

Can you provide more information in the PR description regarding this change as well?

@amrgetment
Copy link
Author

@miquelbeltran it is a mistake, actually I use package info plus not device info plus
updated the description

@amorenew amorenew mentioned this pull request Feb 28, 2024
Closed
miquelbeltran
miquelbeltran reviewed Feb 29, 2024
View reviewed changes
Copy link
Member

@miquelbeltran miquelbeltran left a comment

Choose a reason for hiding this comment

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

This is a bit confusing.

The package already uses web, so the ticket you created seems to be wrong. Can you elaborate on that?

packages/package_info_plus/package_info_plus/lib/src/package_info_plus_web.dart
@@ -4,7 +4,7 @@ import 'package:flutter_web_plugins/flutter_web_plugins.dart';
import 'package:http/http.dart';
import 'package:package_info_plus_platform_interface/package_info_data.dart';
import 'package:package_info_plus_platform_interface/package_info_platform_interface.dart';
import 'package:web/helpers.dart' as web;
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to leave helpers.dart, because we want to support web 0.3

Copy link
Author

Choose a reason for hiding this comment

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

oh, I will revert it back if the PR open again

Copy link
Author

Choose a reason for hiding this comment

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

reviewer made an alternative PR already

packages/package_info_plus/package_info_plus/lib/package_info_plus.dart
@@ -9,7 +9,7 @@ import 'package:package_info_plus_platform_interface/package_info_platform_inter

export 'src/package_info_plus_linux.dart';
export 'src/package_info_plus_windows.dart'
if (dart.library.html) 'src/package_info_plus_web.dart';
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this change?

Copy link
Author

Choose a reason for hiding this comment

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

according to this document you need to use js_interop for conditional import to support WASM,
am I wrong?
https://dart.dev/interop/js-interop/package-web

@vbuberen
Copy link
Collaborator

package_info_plus is already migrated to web package and we need to support versions since 0.3.0.
So I don't see the reason to have this PR. Closing it as we are not going to accept it anyway.

@vbuberen vbuberen closed this Feb 29, 2024
@vbuberen
Copy link
Collaborator

The migration happened a few months ago already #2316

@vbuberen vbuberen mentioned this pull request Feb 29, 2024
Merged
2 tasks
@amrgetment
Copy link
Author

@vbuberen according to this document you need to use js_interop for conditional import
https://dart.dev/interop/js-interop/package-web

image

so are you sure it is compatible with WASM with dart.library.html?

@vbuberen
Copy link
Collaborator

I am sure, because if you would check the link right above your comment you would see that I created another PR with the same change, but took a few minutes to provide a proper description with links instead of what was provided in this PR.
Instead, you are replying to something is already irrelevant, not to mention that tag somebody for no reason, considering that people who leave comments in issue/PR will get notifications anyway.

Screenshot 2024-02-29 at 12 44 30

@amrgetment
Copy link
Author

amrgetment commented Feb 29, 2024 •
edited
Loading

@vbuberen I replied to this

So I don't see the reason to have this PR

we have a different time zones so I replied late as I made this PR night and I checked it noontime

@vbuberen
Copy link
Collaborator

I have just explained to you why you shouldn't tag people and you still continue 🤦🏻

we have a different time zones so I replied late as I made this PR night and I checked it noontime

It doesn't matter. In case PR had a proper description with links (like the one that I created instead) and didn't reference an issue migrate to package:web which is already resolved for this package, nobody would have any questions and the timezone wouldn't matter as reviewers would understand what is the reason for change.

@vbuberen
Copy link
Collaborator

Next time please provide a clear and full description as people might not be in the context of what you have in your head. With that being said locking this discussion as there is nothing to discuss more.

@fluttercommunity fluttercommunity locked as resolved and limited conversation to collaborators Feb 29, 2024
@amrgetment amrgetment deleted the feature/migrate-wasm branch February 29, 2024 11:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@miquelbeltran miquelbeltran miquelbeltran left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Request]: (package_info_plus): migrate to pkg:web from dart:html
3 participants
@amrgetment @miquelbeltran @vbuberen