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

Browser Support #5

Open
Imgkl opened this issue Jan 17, 2024 · 9 comments · Fixed by #6
Open

Browser Support #5

Imgkl opened this issue Jan 17, 2024 · 9 comments · Fixed by #6
Labels
critical enhancement New feature or request

Comments

@Imgkl Imgkl added the enhancement New feature or request label Jan 17, 2024
Imgkl added a commit that referenced this issue Jan 17, 2024
@Imgkl Imgkl linked a pull request Jan 17, 2024 that will close this issue
@Imgkl Imgkl closed this as completed in #6 Jan 17, 2024
Imgkl added a commit that referenced this issue Jan 17, 2024
@Imgkl Imgkl reopened this Jan 26, 2024
@Imgkl
Copy link
Owner Author

Imgkl commented Jan 26, 2024

The fetch_api package is intended for web use, which would explain why it's relying on dart:js. However, this becomes an issue when you try to run your Flutter app on platforms other than the web because the package is not compatible with non-web environments. Just by importing it in the package, seems to cause issue.

Possible Solutions:

  • Creating a new package eventflux_client_manager and do conditional imports based on platform
    • Pros
      • Sounds simple
    • Cons
      • overhead of managing two packages just for sake of web support.
  • Implement a solution from ground up for web
    • Pros
      • highly customisable to my liking and user needs
    • Cons
      • Heavy lift.
  • Explore Dio in favour of http

@Imgkl Imgkl added the critical label Jan 26, 2024
@The-RootCause
Copy link

Any updates on this issue ?
It's very important to start using EventFlux.
Thank you

@Zekfad
Copy link

Zekfad commented May 4, 2024

Hi, I'm fetch_client author.

Was checking usage of package and stumbled upon this issue. We've fixed it in last version, so you can now import client in VM (it will throw UnsupportedError is you try to instantiate it).

As a side note, when you use web only package (which uses js_interop or dart:html) you should do conditional import like in this example from package:http: https://github.com/dart-lang/http/blob/dd31e64685b03a9700cc99d7b7fc637dd60c4d3a/pkgs/flutter_http_example/lib/main.dart#L13-L14

Hope this will help to resolve this!

@Peetee06
Copy link
Contributor

@Imgkl Do I understand correctly, that the fetch_client simply needs to be updated? Could contribute that if you're not already on it. :)

I'm looking into using EventFlux and need browser support.

@Imgkl
Copy link
Owner Author

Imgkl commented Nov 21, 2024

@Peetee06

Please give this a try. I couldn't get the condition import to work or the vm method.

I'm happy to merge at as soon as possible if the web support is working.

@Peetee06
Copy link
Contributor

@Imgkl good, will try it out tomorrow 👍

@Peetee06
Copy link
Contributor

Peetee06 commented Nov 25, 2024

I tested browser support with fetch_client successfully. I used https://sse.dev/test to test the SSE stream.

We have two possible solutions:

1. Instantiating FetchClient inside the connect method on web

This would mean we need to allow users to pass some parameters to connect that the FetchClient has, so users can configure it as needed:

FetchClient({
    this.mode = RequestMode.noCors,
    this.credentials = RequestCredentials.sameOrigin,
    this.cache = RequestCache.byDefault,
    this.referrer = '',
    this.referrerPolicy = RequestReferrerPolicy.strictOriginWhenCrossOrigin,
    this.redirectPolicy = RedirectPolicy.alwaysFollow,
    this.streamRequests = false,
  });

This will require adding a set of enums to EventFlux that we can map to the ones of FetchClient. This includes writing tests for the correct mappings.
It also means we need to update them whenever the API of fetch_client changes in the future. E.g. when an enum value is added or removed.

2. Replacing HttpClientAdapter with BaseClient and documenting the use of fetch_client

This is a little less seamless for users, because they need to add fetch_client to their direct dependencies and configure it themselves. They can then pass it to connect via the httpClient parameter.
It's also a breaking change, because we need to change the httpClient type to BaseClient so we can actually pass in the FetchClient.


I prefer solution 2, because it gives users control over the correct FetchClient parameters, while preventing EventFlux from becoming more complex and error prone because of mapping and passing the parameters through to the FetchClient constructor.

After having realized we need to change HttpClientAdapter to BaseClient which is a breaking change and thinking more from the user's perspective, I think it's actually nicer to go with option 1.

What do you think about these options @Imgkl?

@Imgkl
Copy link
Owner Author

Imgkl commented Nov 26, 2024

Option 1 feels more user-friendly choice, especially for DX. We do need to write tests for mapping and Ideally we should lock the fetch_client version so we have much control over stuffs, If they push breaking changes.

But that said, I was also worrying about the connect method params getting out of hand, if we have to support these enums and params.

FetchClient({
    this.mode = RequestMode.noCors,
    this.credentials = RequestCredentials.sameOrigin,
    this.cache = RequestCache.byDefault,
    this.referrer = '',
    this.referrerPolicy = RequestReferrerPolicy.strictOriginWhenCrossOrigin,
    this.redirectPolicy = RedirectPolicy.alwaysFollow,
    this.streamRequests = false,
  });

Maybe we can have a new model called WebConfig or something, to encapsulate these web specific configs. This would help keep the connect method clean and make the parameters more manageable. Your thoughts?

@Peetee06
Copy link
Contributor

I agree that this is better DX. I'll go ahead and implement it using WebConfig so there is only a single additional parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants