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

Added support for WASM compilation #516

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Added support for WASM compilation #516

wants to merge 6 commits into from
+162 −129

Conversation

pmathew92
Copy link
Contributor

📋 Changes

This PR adds support for WASM compilation

📎 References

#463

🎯 Testing

This has been tested by running the example app with the following command
flutter run -d chrome --web-port 3000 --wasm

@pmathew92 pmathew92 requested a review from a team as a code owner February 18, 2025 06:23
navaronbracke
navaronbracke suggested changes Feb 18, 2025
View reviewed changes
Copy link

@navaronbracke navaronbracke left a comment

Choose a reason for hiding this comment

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

Just a quick review, as I have experience with WASM interop from other projects.
(and this might also affect one of my changes to this plugin, hence the added interest)

@pmathew92
Copy link
Contributor Author

pmathew92 commented Feb 18, 2025 •
edited
Loading

Just a quick review, as I have experience with WASM interop from other projects. (and this might also affect one of my changes to this plugin, hence the added interest)

Hi @navaronbracke Thank you for reviewing the PR .Appreciate you for taking the time to do so

navaronbracke
navaronbracke suggested changes Feb 19, 2025
View reviewed changes
Copy link

@navaronbracke navaronbracke left a comment

Choose a reason for hiding this comment

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

Some minor follow-up comments, but this is looking good!

auth0_flutter/lib/src/web/js_interop.dart Outdated
external String get name;
extension type Auth0ClientInfo._(JSObject _) implements JSObject {
external JSObject? get env;
external String get name;
Copy link

Choose a reason for hiding this comment

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

There is an extra space here

@pmathew92
Copy link
Contributor Author

Hi @navaronbracke , Would you know what would be the best way to modify the test class
https://github.com/auth0/auth0-flutter/blob/main/auth0_flutter/test/web/auth0_flutter_web_test.mocks.dart
Currently I am getting the error
Classes and mixins can only implement other classes and mixins.

  _FakeAuth0Client_0(
    Object parent,
    Invocation parentInvocation,
  ) : super(
          parent,
          parentInvocation,
        );
}```
in the above class declaration as Auth0Client is now an extension type

@navaronbracke
Copy link

Extension type mocking for the new JS types requires some changes.

See the docs at https://dart.dev/interop/js-interop/mock

mockito 5.4.5 did add support for extension types, though, so you can als look into that.

@pmathew92
Copy link
Contributor Author

Hi @navaronbracke I have updated the PR and fixed the failing UTs
One query , we have this check in the web.dart file which will load the correct file when compiled to WASM

    if (dart.library.js_interop) 'web/auth0_flutter_plugin_real.dart';```
 But this will fail for users who don't compile to WASM. Is there any way for this check to consider for non-WASM compiles ?

@navaronbracke
Copy link

navaronbracke commented Feb 26, 2025 •
edited
Loading

dart:html does not work in the new style JS interop, which is why the conditional import should be changed to dart.library.js_interop. The dart:js_interop import is available for both the JS backend & the WASM backend.

Since you updated the minimum Dart & Flutter versions in the pubspec, users that want to compile to JS or WASM, using WASM compatible API's are going to need to migrate to that minimum Flutter version.

So I don't see why this would fail if you are running on Flutter 3.24 and trying to compile to JS. Both JS & WASM should work with the new API's.

Widcket
Widcket reviewed Mar 3, 2025
View reviewed changes
auth0_flutter_platform_interface/pubspec.yaml
sdk: ">=2.17.0 <4.0.0"
flutter: ">=3.0.0"
sdk: ">=3.5.0 <4.0.0"
flutter: ">=3.24.0"
Copy link
Contributor

@Widcket Widcket Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We will need to call out this change in the release notes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is 3.24.0 the minimum Flutter version we can support?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for auth0_flutter/pubspec.yaml.

Widcket
Widcket reviewed Mar 3, 2025
View reviewed changes
auth0_flutter/test/web/extensions/web_exception_extension_test.dart

if (additionalProps != null) {
for (final element in additionalProps.entries) {
setProperty(jsObject, element.key, element.value);
jsObject.setProperty(element.key.toJS, element.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call element.value.toJS here, instead of the the call site in the tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Widcket Widcket Widcket left review comments

@navaronbracke navaronbracke navaronbracke requested changes

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
3 participants
@pmathew92 @navaronbracke @Widcket