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

Consider removing Option from package exports #79

Closed
Mattertorian opened this issue Jan 12, 2024 · 11 comments
Closed

Consider removing Option from package exports #79

Mattertorian opened this issue Jan 12, 2024 · 11 comments

Comments

@Mattertorian
Copy link

Thanks for a great package!

My issue is pretty much the title: the Option class exported by this package conflicts with the Option class from dedicated FP packages like fpdart. Having to hide Option on every import is quite combersome. I understand the internal usage. It would be great if it did not leak.

Keep up the good work!

@Mattertorian Mattertorian changed the title Consider removing Option from the package Consider removing Option from package exports Jan 12, 2024
@GregoryConrad
Copy link
Owner

GregoryConrad commented Jan 12, 2024

Hi! 👋

Unfortunately, that was an explicit design decision since I didn't want to add a dependency on an external package (that could introduce breaking changes whenever they wish), and the workaround for that is just hand-rolling an Option type. Option is required for AsyncValue, since AsyncValue might contain a nullable type and Option is needed to wrap around that.

That being said, I am familiar with fpdart and did consider using their Option when I first built ReArch. Decided against it for the external dependency reason above.

Now, here's what a workaround would look like (that I probably should document in the FAQs...):

// NOTE: This is untested; you may need to tweak this
// rearch_fix.dart
import 'package:rearch/rearch.dart' as rearch;
import 'package:fpdart/fpdart.dart' as fpdart;

export 'package:rearch/rearch.dart' hide Option;

extension IntoFpdartOption on rearch.Option {
  // you can name this whatever you want
  fpdart.Option<T> into<T>() {
    return switch (this) {
      // TODO: make the fpdart.Option here
      rearch.Some(:final value) => throw UnimplementedError(),
      rearch.None() => throw UnimplementedError(),
    };
  }
}

Then import rearch_fix.dart instead of rearch.dart elsewhere in your application.

@GregoryConrad GregoryConrad closed this as not planned Won't fix, can't repro, duplicate, stale Jan 12, 2024
@Mattertorian
Copy link
Author

Thank you for the workaround. I agree on not depending on fpdart or any other external package for an Option/Result type.

Given that you've put a lot though into this project, I imagine that you've considered simply using nullable types instead on an Option type?

  T unwrapOr(T defaultValue) {
    return switch (this) {
      null => defaultValue,
      (final T value) => value,
    };
  }

  T unwrapOrElse(T Function() defaultFn) {
    return switch (this) {
      null => defaultFn(),
      (final T value) => value,
    };
  }

  R? map<R>(R Function(T) mapper) {
    return switch (this) {
      null => null,
      (final T value) => mapper(value),
    };
  }

I see that you like the Rust approach of Option types which I agree with. However, there is also something to be said for keeping with the features of the programming language.

With that said, thank you for your work on this package.

@GregoryConrad
Copy link
Owner

GregoryConrad commented Jan 12, 2024

I imagine that you've considered simply using nullable types instead on an Option type?

Yup, and I really wish it was a nullable type. The issue is if someone has an AsyncValue with a nullable type, e.g., AsyncValue<int?>. If we had an AsyncLoading state, how does one differentiate between a previous state of null or the fact that there wasn't a previous state? You could do a workaround like hasPreviousData, in addition to the previous data as a nullable type, but I choose the Option route because it actually fixes the problem instead of being a workaround. Granted, it can require more code in some situations (doesn't have dedicated language syntax), but I think it's an acceptable compromise.

The AsyncValue with nullable types is just the tip of the iceberg. copyWith with nullable values (and similar functions) are a whole different can of worms--there are so many ugly hacks required for them to work. This, in all, is really a limitation of Dart as a language, especially with how it handles function parameters.

there is also something to be said for keeping with the features of the programming language.

Absolutely! I personally haven't (yet, at least) used fpdart or any similar libraries for this exact reason. Maybe fpdart doesn't break conventions too much 1, but many similar types of packages, across programming languages, fight too much with their languages' conventions.

Footnotes

  1. I do know that fpdart circumvents try/catch with Either, which I'd normally argue is bad, but I hate the whole try/catch system so much that I think Either is warranted. Errors as data is a significant improvement.

@Mattertorian
Copy link
Author

Thank you for your reflections. I see the value of your arguments and would do the same in an internal library. For an external library like rearch I think that hasPreviousData and a named contructor like AsyncLoading.withPreviousData(previousData) would be preferable.

Another alternative is to use a more specific name for the Option class, possibly more closely related to its actual use. I know this breaks with Rust conventions but it would resolve the issue.

At any rate, thanks for the discussion!

@busslina
Copy link

@Mattertorian You can always use a custom Typedef

@Mattertorian
Copy link
Author

@Mattertorian You can always use a custom Typedef

I was not aware of that. Can you give a quick demonstration of how it is done?

@busslina
Copy link

I was not aware of that. Can you give a quick demonstration of how it is done?

rearch_safe.dart:

import 'package:rearch/rearch.dart';

export 'package:rearch/rearch.dart' hide Option;

typedef ROption = Option;

test.dart:

import 'rearch_safe.dart';
import 'package:fpdart/fpdart.dart';

void test(Option valueFpDart, ROption valueRearch) {}

@Mattertorian
Copy link
Author

Thanks for the snippet. I can see that working if put into a separate package that is then depended upon as a proxy for rearch.

@busslina
Copy link

busslina commented Jan 13, 2024

Thanks for the snippet. I can see that working if put into a separate package that is then depended upon as a proxy for rearch.

You can use it directly in your project. Just import rearch_safe.dart instead of package:rearch/rearch.dart everywhere

@Mattertorian
Copy link
Author

You can use it directly in your project. Just import rearch_safe.dart instead of package:rearch/rearch.dart everywhere

True. But it seems less ergonomic when using the Quick Fix (which would provide both rearch and rearch_safe as import options). I do not know any workaround for that.

@busslina
Copy link

True. But it seems less ergonomic when using the Quick Fix (which would provide both rearch and rearch_safe as import options). I do not know any workaround for that.

Yeah, it will offer you all available options: relative paths, absolute paths, package path, ...

I use prefixes for my libs in order to avoid name clashes: fllib, flib, llib, lib, pglib, ... so I do a lot of copy paste. I don't think it's worth it to create a proxy package, but it's up to you :)

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

No branches or pull requests

3 participants